Repository navigation
Conversation
Fixes required by the release checkpoint 1 review, on top of main: - auth: credential mutations are serialized; user password and role replacement is one atomic program, so a failure never deletes the user, its keys or its grants; a recreated user inherits no orphaned keys or per-KG grants; the bootstrap key is issued from durable state, printed and saved only once stored, with a one-time backfill so existing deployments never re-issue it; Argon2 runs before the storage lock. - ws: the outbound fence's final revocation/expiry check and the frame enqueue are atomic on every data-frame path. - sdk-js: the client-side vectorSearch rewrite, the implicit switch to the default KG before a drop and the local-file load are removed; the server-file .load API is restored. - perf-gate: open-loop delta and interference writers; GenBI counts a mutation as converged only when every subscription converged on a successfully evaluated answer. - ci: the perf gate tool checks and the Verified Completions gate run in the full suite at release checkpoints, not on the fast PR gate. - make pre-pr: requires the merge base with origin/main (or PRE_PR_BASE), routes changed paths to their own fast checks in parallel, then runs the perf gate before every push to a PR.
…p key; retire GenBI questions
This was referenced Oct 4, 2026
Closed
jsam
added a commit
that referenced
this pull request
Oct 4, 2026
…ry request (#249) The release-checkpoint performance gate fails on main: cheap_query serial p50 is 1.110x the approved baseline e732cd5 on the 32-vCPU runner (115 -> 126 us). Concurrent latency and throughput did not move. Bisected to #214 (per-connection request pipelining), made worse by #222. Cause: the pipeline runs requests in a `FuturesUnordered`. Its `poll_next` wakes its own task and returns `Pending` after any poll that polled every future in the set. With one request in flight, which is every serial client, that happens on each request's first poll. The connection task wakes itself, tokio schedules that as a yield, and a yield wakes an idle worker to take the task. Each request therefore costs one more context switch (4.0 -> 5.0 per query) and a hand-off of the connection task to another core. #222 added no work per query but made the hand-off dearer: user cycles per query rose 15% at the same instruction count. Fix: keep one never-woken future in the set. After the pipeline's first poll it is never polled again, so no poll reaches every future and the forced yield no longer fires. The yield after futures that wake themselves still applies. Pipelining, ordering, deadlines, cancellation and the credential fence are unchanged. Nothing is added per request: no lock, atomic or allocation. Each connection gets one more set node. Bisect (cheap_query, `?edge(1, Y)`, serial). Server counters per query from perf stat. Ratios are against e732cd5, from an interleaved probe with one pinned server per commit, 40 rounds: | commit | PR | user instr/query | ctx switches/query | p50 ratio | |---|---|---|---|---| | e732cd5 | baseline | 1.282M | 4.0 | 1.000 | | d75ce6e | #208 atomic programs | 1.279M | 4.0 | 0.993 | | 4578fe6 | #215 notification stream | 1.271M | 4.0 | 0.991 | | 71a4d24 | #216 online backup | 1.275M | 4.0 | 1.016 | | 2f621c5 | #200 first-update latency | 1.272M | 4.0 | 1.009 | | e96df5c | #207 | 1.279M | 4.0 | 0.998 | | 4dd8fe9 | #209 shard index | 1.272M | 4.0 | 0.987 | | 277c7f0 | #206 | 1.274M | 4.0 | 1.008 | | 3554c98 | #214 pipelining, deadlines | 1.296M | 5.0 | 1.020 | | 3492a59 | #213 | 1.294M | 5.0 | 1.030 | | 2ba9a5a | #212 provenance | 1.297M | 5.0 | 1.029 | | 9ed61b4 | #204 key expiry | 1.297M | 5.0 | 1.027 | | be35ee3 | #221 | 1.297M | 5.0 | 1.013 | | c98a590 | #222 bounded streaming | 1.298M | 5.0 | 1.047 | | a6fd628 | #219 wildcard fix | 1.297M | 5.0 | 1.061 | | b7f7ad6 | #220 WAL durability | 1.300M | 5.0 | 1.056 | | de54807 | #223 shared standing eval | 1.299M | 5.0 | 1.071 | | 55ec34d | #225 checkpoint-1 fixes | 1.300M | 5.0 | 1.045 | | 23712b8 | main | 1.300M | 5.0 | 1.064 | | this change | | 1.300M | 4.0 | 1.033 | Confirming A/Bs with the gate's own fixture (fresh pinned servers each round, 40 rounds, ratio [95% interval]): - #214 vs its parent 277c7f0: 1.039 [1.027..1.049] - #222 vs its parent be35ee3: 1.025 [1.006..1.044] - A/A 3554c98: 1.010 [1.002..1.025] - e732cd5 -> main: 1.048 [1.028..1.066] - main -> this change: 0.978 [0.958..0.989] - e732cd5 -> this change: 1.021 [1.009..1.044], PASS (budget 1.05) User cycles per query (perf stat, same instruction count): e732cd5 958K, be35ee3 1053K, c98a590 1206K, this change 1006K. Full gate, all fixtures, 20 rounds, servers pinned to CPUs 2-15: - main -> this change: no metric worse. Every required metric is PASS, or INCONCLUSIVE with an interval that includes 1.0. cheap_query p50 0.979 [0.953..1.007]. - e732cd5 -> this change: cheap_query p50 1.029 [1.019..1.072], INCONCLUSIVE on a busy host (load average up to 22); bound_query p50 1.022, PASS. The insert and delta metrics improved since the baseline. The shared host is about 4x slower per query than the CI runner, so the same hand-off is a smaller share of each query here than there. The remaining ~1.7% of user instructions per query since #214 is the pipeline's own per-request bookkeeping: the boxed request future (one 4-8 KiB allocation), its set node, the request control and the in-flight maps.
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
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.
Intent
Do not run CI redundantly; it is okay to move quickly and break things, then run QC at certain checkpoints. Call them release checkpoints, where the entire CI and QC verification runs. To move quickly, run and test things locally, make sure the relevant checks run in parallel before making the commit, and only run the checks that make sense, not everything. The performance policy should be done as a pipeline which is always run before a PR commit. Make sure the reactive expectations are part of the e2e quality checks pipeline. Always follow clean code principles and clean code architecture. After every big PR run QC agents that verify the issue or feature has been fully implemented or resolved. Documentation changes needed also have to always be kept up to date. Never put Claude/AI co-author or generated-with trailers in commits or PRs.
This change is the checkpoint-1 review fixes on top of main, and it supersedes PR #217 (put 'Supersedes #217' in the PR body). Main already contains the release checkpoint 1 items plus API key expiry/rotation (#204), the WAL durability contracts (#220), streamed oversized subscriptions (#222) and shared standing-query evaluation (#223); this change adds only the fixes the checkpoint review required: credential mutations serialized by a dedicated lock (never the storage write lock, so queries, proofs and replay never delay a revocation); user password/role replacement as one atomic program that fails closed on an unknown outcome; one shared user-access cleanup for user drop and user creation (best effort per #220: keys and grants as separate writes, unknown outcome or read-only store aborts) so a recreated username never inherits orphaned keys or grants; bootstrap API key issuance from durable state, printed and saved only once stored, with an idempotent backfill so existing deployments never re-issue it; Argon2 hashing before any lock; an atomic revocation/expiry check at the outbound frame hand-off on #222's send path; removal of the JS SDK's client-side vectorSearch rewrite, the implicit switch to the default KG before a drop and the local-file load (server-file .load restored); open-loop perf-gate writers; GenBI convergence counted only on successfully evaluated answers; CI gating of the perf-gate tool checks and the Verified Completions gate to the release-checkpoint full suite; and make pre-pr, which requires the merge base with origin/main (or an explicit PRE_PR_BASE), routes changed paths to their own fast checks in parallel (reusing unit-test) and then runs the perf gate before every push to a PR. The WAL rollback-failure contract is owned by main's #220 and is not changed here.
What Changed
credential_mutation_tests.rsandtests/credential_revocation_tests.rs.vectorSearchon the client; it uses the server'stop_k/within_radiusqueries again.dropKnowledgeGraphno longer switches to the default KG on its own first.load()once again sends a server-side.load <path> [mode]instead of reading a local file. Perf-gate writers now run open-loop. GenBI convergence counts only answers that were evaluated successfully, and retired questions are left out of it.ci.yml, plus the deletedverified-completions.yml) and into the release-checkpoint full suite. A newmake pre-prtarget requires a merge base withorigin/mainor an explicitPRE_PR_BASE. It runs only the fast checks for the paths that changed, in parallel and reusingunit-test, then runsmake perf-gate. Its self-tests are inscripts/test_pre_pr.py. The SDK and auth guides, the command reference, CONTRIBUTING, TESTING.md and the PR template are updated to match.Supersedes #217
Risk Assessment
✅ Low: The round-2 fix is narrow and correct:
take_activityreturns every subscription, soarrivalsis empty only when every subscription is retired (or there are none), and such a mutation is no longer counted as converged. The round-1 fixes match the operator's decisions: the bootstrap key is stored in one atomic program, then saved, then printed; unknown users are rejected undercredential_writes; the password is hashed before any lock is taken; and the pre-pr gate runs its own tests when the Makefile or scripts change.Testing
I drove a real, isolated inputlayer-server through a Node WebSocket client. Unknown users are rejected for ACL grants. Dropping a user ends their password and key sessions with credential_revoked and removes their grants and keys, so a recreated user gets neither. A password change ends only the sessions on the old password. Revoking a key with an active subscription delivers no pushes after the revoked notice. Bootstrap never re-issues the key on restart and never prints a key it could not save. A data dir bootstrapped by the base binary gets a one-time backfill and keeps its working key. The make pre-pr base guard and routing selftest pass. A 1-round quick perf-gate run exercised the open-loop writers on both servers; its INVALID verdict is expected for a run that small. Of 75 JS SDK tests run against the live server, 74 pass. The one failure (expects not_found, gets unsupported) happens identically on the base server and comes from a test this change does not touch. Four scenarios were covered only by unit tests (credential_mutation_tests, credential_revocation_tests, perf-gate genbi tests), which all passed, but they are recorded as untested live. This change has no UI, so there are no screenshots; the evidence is CLI and protocol transcripts.
.kg acl grant default ghost viewer-> error "User 'ghost' not found".apikey listshows only bootstrap, and the old key and password are rejec…cargo test --lib credential_mutation_tests. Keys are always created f…cargo test deltascheduled_writes_do_not_wait_for_acknowledgementscargo test genbiunit tests. The live GenBI benchmark reads the external genbi-trust suite from GENBI_TRUST_DIR, which thi…Evidence: Live credential scenarios transcript (unknown-user grant, drop/recreate cleanup, password revocation, key revoke fence)
Evidence: Bootstrap restart: no re-issue, credentials.toml unchanged
Evidence: Bootstrap with unwritable credentials file: key never printed or saved unstored
Evidence: Base-binary data dir upgraded to new server: idempotent bootstrap-key backfill
Evidence: make pre-pr base guard (live)
Evidence: pre-pr routing selftest (real make, stubbed tools)
Evidence: Rust credential mutation / outbound fence / revocation tests
Evidence: perf-gate GenBI convergence tests
Evidence: perf-gate quick delta/interference smoke report (base vs candidate)
Evidence: JS SDK integration tests against live server
~/.no-mistakes/evidence/01M42HZ2NXXCM6A6ZY6SW2G3CF/js_partial_failure_on_base_server.txt)Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 6 issues found → auto-fixed (2) ✅
perf-gate/src/genbi/agent.rs:220-divergednow counts a subscription as diverged when its question has no truth entry or when it has aSubscriptionError. Neither state ever clears.requery(scenario.rs:377-390) retires a question that fails to evaluate after agents subscribed to it: it goes intolive.failedand never comes back intotruth, but the agents' subscription to it stays.sub.erroris set once and never reset (agent.rs:203). Concrete case: question q3 evaluates on the initial query, then fails a later requery (for example a truncated result as data grows). From then on, every remaining mutation (1) blocks indrain_until(deadline, |a| a.diverged(&truth).is_empty())(scenario.rs:316) until the fulloptions.deadline, and (2) getsconvergence_time= None, soconverged_mutationsstops counting even when every successfully evaluated answer converged. The intent says convergence is counted 'only on successfully evaluated answers', which reads as excluding answers that could not be evaluated, not as letting one failure block convergence for the rest of the run. Decide which is meant. If exclusion is meant, skip subscriptions whose id is inlive.failed(and decide how a subscription error should count) instead of treating missing truth as divergence.src/protocol/handler.rs:1479-create_usernow callsdelete_user_accessbefore inserting the user row, which deletes every API key and KG grant held under that username. Neither.kg acl grant(handler.rs:1809, no user-existence check) nor.apikey create(api_keys.rs:185, no owner check) rejects an unknown user. So this documented-style sequence now silently loses access:.kg acl grant mydb alice viewer(or.apikey create k alice), then.user create alice .... The grant or key that was just created is deleted with no message, and the plaintext key the admin was given stops working. The intent requires that a recreated username never inherits orphaned keys or grants, and a grant made in advance looks the same as an orphaned one. The smallest honest remedy changes product behaviour, so it needs a decision: either reject grants and key creation for unknown users (undercredential_writes, the earliest shared boundary), or document that access granted before a user exists is discarded when the user is created.src/protocol/handler.rs:1190- The intent requires the bootstrap API key to be 'printed and saved only once stored'. The change prints only after storing, but it still writes the generated key to credentials.toml (to_persist.save, line 1190) beforecreate_user/store_bootstrap_key(lines 1227-1243). It removes the key only if storing returns an error (line 1247). Crash window:store_bootstrap_keywrites thebootstrap_keysrecord first, then the key. A crash between those two writes leaves the admin user and the record in storage and the key in credentials.toml, but the key was never stored. On the next boothas_usersis true and nothing re-issues or cleans it up, so the file holds a key that never works. The rollback save (line 1247) can also fail, with the same result. Remedy within intent: save the password first as now, and add the API key to credentials.toml only afterstore_bootstrap_keysucceeds.src/protocol/handler.rs:1222- The intent requires 'Argon2 hashing before any lock'.seed_admin_credentialstakescredential_writesand the storage read guard at the top (lines 1099-1100), then runsauth::hash_password(&password)(line 1222) while holding both. This is startup-only, so contention is unlikely, but it contradicts the stated constraint that the request paths (handle_user_create,handle_user_password) now follow. Either hash before taking the locks, or confirm that bootstrap is exempt.scripts/test_pre_pr.py:1- The new behavioural tests formake pre-pr(routing, merge-base selection, failing on a missing base, parallelism, perf gate last) are not run by any Makefile target or workflow; nothing in Makefile or .github/workflows refers to test_pre_pr.py or runs unittest. A change to the pre-pr recipe can therefore break these tests without anyone noticing. Run them from a target, e.g. route Makefile and scripts changes in pre-pr topython3 -m unittest scripts/test_pre_pr.py, and/or add it to the full suite.src/protocol/handler.rs:1580- The comment inhandle_user_dropsays keys, their times and KG grants are deleted 'in one commit'.delete_user_access, as its own doc says, deletes keys and grants as separate writes and keeps going after a transient failure of one, which the intent also requires. Reword the comment so it matches: separate best-effort writes, and the user row only after both succeed.🔧 Fix applied.
1 warning still open:
perf-gate/src/genbi/scenario.rs:446- Introduced by the round-1 fix 'genbi-retired-question-blocks-convergence'.record_deltasnow leaves retired subscriptions out ofarrivals(lines 446-449). When every subscription in a scenario belongs to a retired question,arrivalsis empty, andconvergence_time(line 502) returnsSome(Duration::ZERO)because no subscription diverged and no arrival is late. Each later mutation therefore addsops += 1toconverged_mutationswith 0 µs elapsed, which inflates the rate even though nothing was measured. Concrete case: a scenario with a single question q0 whose re-query later truncates as data grows. q0 goes intolive.failed, and every remaining mutation counts as converged instantly. The default verdict also passes, becauseunconvergedcounts onlyDivergedandQueryFailedfails a check only in strict mode. The operator decided that retired questions should not count as diverged. That decision does not make 'no live subscriptions measured' count as convergence. Fix: skip counting the mutation when it had subscription activity but every entry was retired. For example, track whether any non-retired subscription was seen, and returnNoneor skip the rate update when none was. Then add a single-question case tolater_mutations_converge_after_a_question_is_retired.🔧 Fix applied.
✅ Re-checked - no issues remain.
packages/inputlayer-js/tests/integration.test.ts:846- Integration test 'a partial failure names each failed statement' expects code not_found for.rel drop no_such_relbut the server returns unsupported. It fails the same way on the base-commit server, and this change does not touch the test, so it was already failing. It only shows up when INPUTLAYER_TEST_SERVER is set.src/protocol/handler.rs:1194- With a supplied password and a newly generated bootstrap key, bootstrap logs 'Auth bootstrap: reusing credentials from <path>' even though nothing was reused; the key was just generated. The log is cosmetic and behaviour is correct; the stale branch condition comes from the reordered save logic..kg acl grant default ghost viewer-> error "User 'ghost' not found".apikey listshows only bootstrap, and the old key and password are rejec…cargo test --lib credential_mutation_tests. Keys are always created f…cargo test deltascheduled_writes_do_not_wait_for_acknowledgementscargo test genbiunit tests. The live GenBI benchmark reads the external genbi-trust suite from GENBI_TRUST_DIR, which thi…Builtcargo build --bin inputlayer-server --bin inputlayer-clientand ran the server against throwaway data dirs in /tmp, outside any config.tomlLive Node WebSocket driver (scen_creds.mjs): used the bootstrap key, ran.kg acl grant default ghost viewer, user create/grant/key/drop/recreate,.user password,.user role, and.apikey revokewith an active subscription and writes after the revokeLive restart on the same data dir: checked credentials.toml sha256 and the bootstrap log lines, then tried.user drop adminLive boot withINPUTLAYER_HTTP__AUTH__CREDENTIALS_FILEin a read-only dir, once with a supplied password and once with a generated oneBuilt the base commit 2b6b371 server, bootstrapped a data dir with it, then booted the new server on that data dir twice (backfill, then a second restart)make pre-pr PRE_PR_BASE=does-not-existandmake pre-pr PRE_PR_BASE=HEAD(live base guard)python3 -m unittest scripts/test_pre_pr.py -v(runs the real Makefile with stub tools)cargo test --lib credential_mutation_tests,cargo test --lib ws::outbound,cargo test --test credential_revocation_testscargo test genbiandcargo test deltain perf-gateperf-gate run --profile quick --rounds 1 --fixtures delta_single,interference: base debug server vs new debug server, thenperf-gate comparenpx vitest run tests/integration.test.ts tests/statement-errors.test.tsagainst the live new server (INPUTLAYER_TEST_SERVER)Re-ran the failing JS test-t 'partial failure names'against the base-commit server✅ **Document** - passed
✅ No issues found.
packages/inputlayer-js/src/knowledge-graph.ts- JS SDK type check (npm run typecheck/tsc --noEmit) could not run:tscis not installed because packages/inputlayer-js has no node_modules, and installing dependencies was out of scope. The changed client.ts, knowledge-graph.ts and test files were not type-checked in this pass. Rust fmt-check and clippy (-D warnings) passed for the workspace and perf-gate, and scripts/test_pre_pr.py compiles.✅ **Push** - passed
✅ No issues found.