| Comment: | fix(sync): derive the project code, and write artifact visibility inside the storing transaction (#204)
* fix(sync): derive the project code from the repo instead of panicking (#199) Supplying User/Password made SyncOpts.ProjectCode mandatory in practice: buildLoginCard passed the caller's raw value straight to computeLogin, which asserts it is non-empty, so an authenticated Sync on a repo whose project code the caller had not repeated took the process down. Nothing in the public API signalled that a password made the field required, and a server embedding the library died with it. buildRequest already fell back to the repo's stored project-code for the push/pull cards; that fallback was local to one function, which is exactly how the login path drifted out of step with it. Resolve the code once when the session is built — caller value first, the repo's own config second — and let buildRequest and buildLoginCard both read it. A code that genuinely cannot be determined now returns a descriptive error from the site that needs it rather than panicking. Sync's deferred result-not-nil assertion fired during panic unwinding, where err and result are both nil, replacing the original cause with its own message. The invariant only holds on a normal return, so it no longer asserts while a panic is in flight. * fix(sync): write received artifacts' visibility inside the storing transaction (#200) Receiving an artifact cost three write transactions, not one. The blob was stored in a transaction, its dephantomize cascade committed another, and then applyPrivateStatus re-derived the rid on the pool and issued a bare DELETE FROM private -- an implicit transaction of its own, with its own BEGIN IMMEDIATE and fsync, for a row that a transaction microseconds earlier had guaranteed did not exist. Every igot card announcing an artifact already held cost one more. Measured against a real fossil peer: 1509 statements executed outside a transaction to receive 1504 artifacts. That is minutes of pure commit overhead on a large repository -- roughly 200,000 write transactions for a 67k-artifact pull -- and it is fragile as well as slow. Each of those writes contests the database's single write lock independently, so N artifacts means N chances to lose that contest and fail outright rather than one chance to wait at a single BEGIN. Visibility now travels with the content it describes: storeReceivedFile takes the visibility a private card announced and records it inside the transaction that stores the blob, using the rid that transaction already has. The status is now atomic with the content rather than a separate write that could be interrupted between the two. The igot path buffers its updates and writes them once per round. The same three-transaction shape on the server's receive path goes with it. The same real-fossil pull now executes 2 statements outside a transaction for those 1504 artifacts, down from 1509. db.DB.PoolExecs makes that budget assertable, which is how the regression test pins O(rounds) rather than O(artifacts) without needing a large corpus. On the failure originally reported: the "database is locked" error came from a harness that copied a live SQLite database out from under a running server without its WAL, so that specific lock holder is not a product defect and this does not claim to fix it. The transaction storm above is a real performance and robustness defect on its own evidence, and is what this changes. * test(sync): pin the transaction budget, not just writes outside a transaction The budget tests measured PoolExecs, which counts statements issued outside a transaction. That pins one half of the defect and misses the other: moving a per-artifact write from the pool into its own WithTx leaves PoolExecs unchanged while every artifact still pays a BEGIN IMMEDIATE and an fsync, and still contests the single write lock on its own. That is the cost issue #200 actually measured -- roughly three write transactions per received file, about 203,000 for one clone-sized pull. Verified by mutation: reverting the batched visibility flush to one transaction per update left both budget tests green. They now count transactions too, via a new db.DB.WriteTxns counter, and the same mutation fails with "began 51 write transactions for 50 igot cards over 1 rounds; want <= 4". Also stop the contention test from starving the pull it is meant to observe. The rival connection re-acquired the write lock immediately on release, so on a loaded machine it could hold the pull's single BEGIN past busy_timeout and fail the test even when the batching under test is perfect -- which is what happened on CI. It now yields between bursts, modelling another process doing occasional writes rather than testing SQLite's fairness. |
|---|---|
| Timelines: | family | ancestors | descendants | both | trunk |
| Files: | files | file ages | folders |
| SHA3-256: |
18bfbc166ced1eb5f985c0f3ed9a1b3a |
| User & Date: | [email protected] on 2026-07-27 23:31:22.000 |
| Other Links: | manifest | tags |
|
2026-07-27
| ||
| 23:37 |
ci: gate the js/wasm build on every PR (#201) (#205)
test.yml had zero references to wasm or GOOS=js, so the WebAssembly build was never compiled on any leg. Browser support is the reason the ncruces driver is in the tree, and a break there would have shipped undetected -- including in db/config_wasm.go, whose js-tagged wasmPragmaOverrides() no job compiled at all. Adds a build-only job covering 31 of the main module's 33 js-resolvable packages plus the ncruces driver submodule. The package set comes from go list rather than a hand-written list so new packages are gated by default. Two main packages are excluded, both for the same structural reason: cmd/libfossil and examples/embed-repo-api import the modernc driver directly in their own main.go, and modernc.org/libc has no js/wasm build. internal/stash and internal/undo declare //go:build !js in source and are dropped by go list itself. Building under -tags test_ncruces keeps internal/testdriver in the gate by pointing it at the driver that can actually target WASM. Running tests under WASM needs a browser harness (wasmbrowsertest) and is deliberately out of scope; compilation catches the realistic failure mode. check-in: 10abca922c user: [email protected] tags: trunk | |
| 23:31 |
fix(sync): derive the project code, and write artifact visibility inside the storing transaction (#204)
* fix(sync): derive the project code from the repo instead of panicking (#199) Supplying User/Password made SyncOpts.ProjectCode mandatory in practice: buildLoginCard passed the caller's raw value straight to computeLogin, which asserts it is non-empty, so an authenticated Sync on a repo whose project code the caller had not repeated took the process down. Nothing in the public API signalled that a password made the field required, and a server embedding the library died with it. buildRequest already fell back to the repo's stored project-code for the push/pull cards; that fallback was local to one function, which is exactly how the login path drifted out of step with it. Resolve the code once when the session is built — caller value first, the repo's own config second — and let buildRequest and buildLoginCard both read it. A code that genuinely cannot be determined now returns a descriptive error from the site that needs it rather than panicking. Sync's deferred result-not-nil assertion fired during panic unwinding, where err and result are both nil, replacing the original cause with its own message. The invariant only holds on a normal return, so it no longer asserts while a panic is in flight. * fix(sync): write received artifacts' visibility inside the storing transaction (#200) Receiving an artifact cost three write transactions, not one. The blob was stored in a transaction, its dephantomize cascade committed another, and then applyPrivateStatus re-derived the rid on the pool and issued a bare DELETE FROM private -- an implicit transaction of its own, with its own BEGIN IMMEDIATE and fsync, for a row that a transaction microseconds earlier had guaranteed did not exist. Every igot card announcing an artifact already held cost one more. Measured against a real fossil peer: 1509 statements executed outside a transaction to receive 1504 artifacts. That is minutes of pure commit overhead on a large repository -- roughly 200,000 write transactions for a 67k-artifact pull -- and it is fragile as well as slow. Each of those writes contests the database's single write lock independently, so N artifacts means N chances to lose that contest and fail outright rather than one chance to wait at a single BEGIN. Visibility now travels with the content it describes: storeReceivedFile takes the visibility a private card announced and records it inside the transaction that stores the blob, using the rid that transaction already has. The status is now atomic with the content rather than a separate write that could be interrupted between the two. The igot path buffers its updates and writes them once per round. The same three-transaction shape on the server's receive path goes with it. The same real-fossil pull now executes 2 statements outside a transaction for those 1504 artifacts, down from 1509. db.DB.PoolExecs makes that budget assertable, which is how the regression test pins O(rounds) rather than O(artifacts) without needing a large corpus. On the failure originally reported: the "database is locked" error came from a harness that copied a live SQLite database out from under a running server without its WAL, so that specific lock holder is not a product defect and this does not claim to fix it. The transaction storm above is a real performance and robustness defect on its own evidence, and is what this changes. * test(sync): pin the transaction budget, not just writes outside a transaction The budget tests measured PoolExecs, which counts statements issued outside a transaction. That pins one half of the defect and misses the other: moving a per-artifact write from the pool into its own WithTx leaves PoolExecs unchanged while every artifact still pays a BEGIN IMMEDIATE and an fsync, and still contests the single write lock on its own. That is the cost issue #200 actually measured -- roughly three write transactions per received file, about 203,000 for one clone-sized pull. Verified by mutation: reverting the batched visibility flush to one transaction per update left both budget tests green. They now count transactions too, via a new db.DB.WriteTxns counter, and the same mutation fails with "began 51 write transactions for 50 igot cards over 1 rounds; want <= 4". Also stop the contention test from starving the pull it is meant to observe. The rival connection re-acquired the write lock immediately on release, so on a loaded machine it could hold the pull's single BEGIN past busy_timeout and fail the test even when the batching under test is perfect -- which is what happened on CI. It now yields between bursts, modelling another process doing occasional writes rather than testing SQLite's fairness. check-in: 18bfbc166c user: [email protected] tags: trunk | |
| 22:04 |
fix: derive leaf by canonical's branch rule, in both the sweep and the commit path (#197)
* fix: derive leaf from fossil's branch rule and bound the deferral rollup Leaf derivation dropped 66% of rows on the real Fossil repository (487 of 1,426) because both leaf paths treated any parent link as retiring a leaf. Canonical fossil retires a check-in only when a child carries the *same* branch name, with an absent branch tag reading as "trunk" (src/leaf.c: is_a_leaf / leaf_check / leaf_rebuild all share that predicate and none consult plink.isprim). Verified against fossil 2.28 itself: after a branch commit off the trunk tip, both the incremental table and `fossil rebuild` keep the trunk tip as a leaf even though its only child is its primary child, and a branch tip merged into trunk stays a leaf as well. Two code paths were wrong, in the same way: - repairLeafTable excluded every `pid` in plink, so merged branch tips disappeared -- the class that accounts for 939 of the corpus's leaves. - Checkin deleted every parent from leaf, so committing onto a new branch silently retired the branch point. Both now share leafCheck/repairLeafTable in internal/manifest/leaf.go. The recompute also has to run after tag propagation rather than before it: a check-in that declares no branch of its own only has a branch value once propagation has written it, and comparing against a missing value made every inherited-branch tip look like a leaf. Applying the new predicate to the real repository's own event/plink/tagxref yields 1,428 against fossil's 1,426, and every one of the six differing rows is fossil's incremental bookkeeping rather than its predicate: 2 are phantom rids fossil's leaf_check inserts as plink parents of check-ins it linked, and 4 are parentless check-ins with no branch tag, which nothing ever enqueues for a leaf check. Running fossil's own leaf_rebuild() on that repository gives 1,494, so it is leaf_check, not leaf_rebuild, that the shipped table reflects. Also caps the deferral rollup's identifier lists (issue #194). A clone deferring 180 check-ins on 417 missing blobs rendered a single ~28 KB log line; the counts stay exact at INFO and the lists print eight entries plus an "...and N more" so nothing is dropped without saying so. Fixes #189 Fixes #194 * chore: retrigger code scanning after branch restore check-in: df495ba69e user: [email protected] tags: trunk | |