publish.sh uploaded a whole repo tree with a single aws s3 sync, giving
no control over upload order. An F-Droid repo is a signed index plus the
files it names, and an interrupted publish could land the index before an
APK it references -- breaking the repo for every client until the upload
finished or was rolled back (#5). Credentials were only checked for being set, not for being usable, which is exactly how a partial upload can
start in the first place.
Separately, nothing enforced one shared rule for computing a monotonic versionCode across CI and a manual build (#6). While implementing that,
bit pointed out (appended to #6 mid-implementation) that the clamp next-version-code.sh uses is monotonic against observed state, not
monotonic by construction: two jobs starting in the same minute can both
read the same published value and race a whole build to publish two
different APKs under the same versionCode.
What changed
publish.sh uploads in two passes, unconditionally: every non-index
file first, then index-v1.json(.asc)/index-v2.json/entry.json/ entry.jar (top-level names only) last. An interruption anywhere in pass
1 leaves the repo describing the old state a little longer; it can never
leave an index naming files that 404.
Credentials are verified with one real authenticated call
(head-bucket) before either pass, not just checked for being non-empty.
New next-version-code.sh (+ stdlib-only next_version_code.py): one
shared rule -- candidate = minutes since 2026-01-01T00:00:00Z, clamped to
strictly exceed whatever is already published for that applicationId --
called by both CI and a manual build.
publish.sh refuses to overwrite an already-existing APK object,
checked before pass 1. This can't prevent the wasted build behind a
same-minute race; it converts the collision from a silent, permanent
wrong state (two different APKs claiming one versionCode) into a loud
failure at the last moment the information is reliable, as required
alongside the clamp.
See the expanded "One versionCode rule" and "Publish ordering and safety"
sections in the README for the full reasoning, including why "monotonic by
construction" was wrong as previously written and is not claimed again.
Testing
Offline: 83 assertions, 0 failed, 0 skipped, across 7 suites (bash run_tests.sh), including two new ones:
test_next_version_code.py -- pure stdlib, no nix: no prior build, lower/
equal/higher prior (clock-went-backwards), several versions, a version
with no versionCode, an application id missing entirely (not an error),
and unreadable/malformed indexes (fail loudly, nothing on stdout).
test_next_version_code.sh -- the real CLI through with-nixpkgs.sh:
argument count, a local index file end to end, a missing application id,
a missing/malformed file, and an unreachable URL (closed port).
test_publish.sh gained: dry-run assertions that the two commands are
printed in order (files, then index) with the right --exclude/ --include; a real invocation against a closed port proving the failure
happens during credential verification, before any upload; and a new
local HEAD-only S3 stub (test_s3_stub.py, pure stdlib) proving the
credential check is a genuine authenticated call (a wrong bucket is
rejected) and that an already-published APK is refused before any upload,
with a negative control proving a non-colliding APK does not trip it.
Live, against fdroid-weblib-dev: 34 assertions, 0 failed (the 27
offline ones for that file, plus 7 that only run against a real bucket):
a real publish, an already-published APK refused with its bytes unchanged
afterwards, and -- the hard part, since the offline suite structurally
cannot see the order two real uploads land in -- the index proven genuinely
absent from the bucket while the file object was already there, observed
from insidepublish.sh's own process via a same-process synchronization
seam (PUBLISH_SH_TEST_HOOK_BETWEEN_PASSES) rather than a poll racing a
background upload, which is exactly the flakiness #5 warned against.
Bucket left empty after every run (verified by listing it before and after).
Every change here was also fault-injected and reverted to confirm the new
tests actually catch it: removing the two-pass filtering (5 failures),
removing the credential check (4 failures), and removing the APK-collision
check (3 of 4 related assertions failing, the 4th coincidentally still
passing for an unrelated reason -- noted in the test comments). Full detail
in the session report, not reproduced here.
What I did not do
Did not add a concurrency: group to serialise publishes -- #6 explicitly
asked not to rely on that, since whether Gitea 1.25.2 honours it is
unverified.
Did not distinguish "the same job retrying after a network drop" from "a
genuine concurrent collision" in the APK-existence check -- both look
identical at the object level without hashing every object, which was
judged not worth the complexity here. Documented as an accepted cost in publish.sh.
## Problem
`publish.sh` uploaded a whole repo tree with a single `aws s3 sync`, giving
no control over upload order. An F-Droid repo is a signed index plus the
files it names, and an interrupted publish could land the index before an
APK it references -- breaking the repo for every client until the upload
finished or was rolled back (#5). Credentials were only checked for being
*set*, not for being *usable*, which is exactly how a partial upload can
start in the first place.
Separately, nothing enforced one shared rule for computing a monotonic
`versionCode` across CI and a manual build (#6). While implementing that,
bit pointed out (appended to #6 mid-implementation) that the clamp
`next-version-code.sh` uses is monotonic against *observed* state, not
monotonic by construction: two jobs starting in the same minute can both
read the same published value and race a whole build to publish two
different APKs under the same `versionCode`.
## What changed
* **`publish.sh` uploads in two passes, unconditionally**: every non-index
file first, then `index-v1.json(.asc)`/`index-v2.json`/`entry.json`/
`entry.jar` (top-level names only) last. An interruption anywhere in pass
1 leaves the repo describing the old state a little longer; it can never
leave an index naming files that 404.
* **Credentials are verified with one real authenticated call**
(`head-bucket`) before either pass, not just checked for being non-empty.
* **New `next-version-code.sh`** (+ stdlib-only `next_version_code.py`): one
shared rule -- `candidate = minutes since 2026-01-01T00:00:00Z`, clamped to
strictly exceed whatever is already published for that `applicationId` --
called by both CI and a manual build.
* **`publish.sh` refuses to overwrite an already-existing APK object**,
checked before pass 1. This can't prevent the wasted build behind a
same-minute race; it converts the collision from a silent, permanent
wrong state (two different APKs claiming one `versionCode`) into a loud
failure at the last moment the information is reliable, as required
alongside the clamp.
See the expanded "One versionCode rule" and "Publish ordering and safety"
sections in the README for the full reasoning, including why "monotonic by
construction" was wrong as previously written and is not claimed again.
## Testing
**Offline: 83 assertions, 0 failed, 0 skipped**, across 7 suites (`bash
run_tests.sh`), including two new ones:
* `test_next_version_code.py` -- pure stdlib, no nix: no prior build, lower/
equal/higher prior (clock-went-backwards), several versions, a version
with no `versionCode`, an application id missing entirely (not an error),
and unreadable/malformed indexes (fail loudly, nothing on stdout).
* `test_next_version_code.sh` -- the real CLI through `with-nixpkgs.sh`:
argument count, a local index file end to end, a missing application id,
a missing/malformed file, and an unreachable URL (closed port).
* `test_publish.sh` gained: dry-run assertions that the two commands are
printed in order (files, then index) with the right `--exclude`/
`--include`; a real invocation against a closed port proving the failure
happens during credential verification, before any upload; and a new
local HEAD-only S3 stub (`test_s3_stub.py`, pure stdlib) proving the
credential check is a genuine authenticated call (a wrong bucket is
rejected) and that an already-published APK is refused before any upload,
with a negative control proving a non-colliding APK does not trip it.
**Live, against `fdroid-weblib-dev`: 34 assertions, 0 failed** (the 27
offline ones for that file, plus 7 that only run against a real bucket):
a real publish, an already-published APK refused with its bytes unchanged
afterwards, and -- the hard part, since the offline suite structurally
cannot see the order two real uploads land in -- the index proven genuinely
absent from the bucket while the file object was already there, observed
from *inside* `publish.sh`'s own process via a same-process synchronization
seam (`PUBLISH_SH_TEST_HOOK_BETWEEN_PASSES`) rather than a poll racing a
background upload, which is exactly the flakiness #5 warned against.
Bucket left empty after every run (verified by listing it before and after).
Every change here was also fault-injected and reverted to confirm the new
tests actually catch it: removing the two-pass filtering (5 failures),
removing the credential check (4 failures), and removing the APK-collision
check (3 of 4 related assertions failing, the 4th coincidentally still
passing for an unrelated reason -- noted in the test comments). Full detail
in the session report, not reproduced here.
## What I did not do
* Did not add a `concurrency:` group to serialise publishes -- #6 explicitly
asked not to rely on that, since whether Gitea 1.25.2 honours it is
unverified.
* Did not distinguish "the same job retrying after a network drop" from "a
genuine concurrent collision" in the APK-existence check -- both look
identical at the object level without hashing every object, which was
judged not worth the complexity here. Documented as an accepted cost in
`publish.sh`.
Closes#5Closes#6
An F-Droid repo is a signed index plus the files it names, and a partial
upload could previously leave them disagreeing: aws s3 sync walks the tree
in its own order, so an interrupted publish could land the index before the
APKs it names, breaking the repo for every client until the upload finished
or was rolled back. publish.sh now uploads in two passes, unconditionally --
every non-index file first, the index (index-v1.json(.asc), index-v2.json,
entry.json, entry.jar) last -- so an interruption anywhere in pass 1 simply
leaves the repo describing the old state a little longer, never a mixture.
Credentials are also verified with one real authenticated call
(head-bucket) before either pass, rather than merely checking the
environment variables are non-empty, since an unusable-but-present
credential failing halfway through is exactly how a partial upload happens.
next-version-code.sh is the one implementation of the versionCode rule
(candidate = minutes since 2026-01-01Z, clamped to strictly exceed whatever
is already published for that applicationId), shared by CI and manual
builds so there is exactly one place this can drift. bit pointed out
(appended to #6) that the clamp is monotonic against *observed* state, not
monotonic by construction: two jobs starting in the same minute can both
read the same published value and race to publish the same versionCode on
different APKs. Nothing can prevent that wasted build, so publish.sh now
also refuses to overwrite an already-existing APK object before pass 1 --
turning the collision from a silent, permanent wrong state into a loud
failure at the last moment the information is reliable.
Tested offline (83 assertions: pure-stdlib suites for both scripts, the
real CLIs through with-nixpkgs.sh, and a small local HEAD-only S3 stub
proving the credential check is a real authenticated call and the
APK-collision refusal fires only when it should) and live against
fdroid-weblib-dev (7 more: a real publish observed mid-flight via a
same-process synchronization hook to prove the index is genuinely absent
while the file is already present, both objects reading back
byte-identical, and a second publish refused against a real
already-published APK with its bytes left unchanged).
Co-authored-by: bit <bit@das-labor.org>
claude
requested review from bit 2026-09-09 08:01:47 +00:00
Cold re-read. Two things before this can merge, one of them mine to decide.
Re-measured here: offline 83 passed, 3 skipped, 0 failed. Secret scan of the diff is clean — the only credential-shaped strings are the deliberate AKIAFAKETESTKEY fakes. Public-repo constraint holds.
The publish work is good. The index filename list is right and, better, reasoned: matching index-v1.json(.asc), index-v2.json, entry.json and entry.jaronly at the top level, with a note that a nested diff/<ts>.json is not "the index" and is only ever fetched by a client that already holds the previous one. That is the kind of distinction that would have been got wrong by pattern-matching on index*.
Also right: the credential check makes a realhead-bucket call rather than testing that variables are non-empty, which is exactly the "set but not usable" case bit asked for.
This evals an environment variable, in a script that runs with the release signing key and S3 credentials in its environment, in a public repository. The comment says "never set by a real caller", and that is true today — but it is a documented, discoverable code path that turns any env-var influence into arbitrary code execution at exactly the worst moment.
I do not think the threat model makes it likely. I think it is unnecessary, which is the better argument.
The alternative is a feature rather than a seam: split the two passes into separately invokable steps — --files-only and --index-only. Then ordering is testable by construction (run pass 1, assert the bucket state, run pass 2) with no hook, no eval, and no test-only branch in production code.
It is also independently useful: an interrupted publish can be resumed at the index step without re-uploading every APK, and a caller who wants to stage now and commit later can.
2. The granularity is still open, and this implements minutes
next_version_code.py uses minutes since 2026-01-01. That was the state when you started, but bit and I have since worked through it on weblib-viewer#15 and the requirement changed: bit wants every build kept, which makes collisions something to render impossible rather than resolve.
My proposal there is 5-second granularity — 340 years of headroom, and a collision requires two builds starting within the same 5 seconds. bit has not answered yet, so this is not a change to make now; it is a one-constant change when they do.
Worth noting the clamp survives either way, but its justification changes: not concurrency any more, but a clock stepping backwards and rebuilding an older commit. The comments should say that when the constant lands.
Well handled, and worth keeping
versionCode read from packages.<appId>.versions.*.manifest.versionCode, verified against fdroidserver's own index.py rather than assumed from the JSON's shape.
The local HEAD-only S3 stub, which makes the credential and collision checks testable offline. That is the difference between a check that is tested and one that is only exercised when someone has credentials.
Finding, while writing the live test, that publish.sh exports the Garage checksum variables only into its own child — so the test's own direct write hit the exact InvalidRequest from #3. Fixed in the test and commented. That is the bug biting from a new direction, and catching it in the test rather than in production is the whole point of having a live test.
Honest reporting that one of four collision assertions stayed green for an unrelated reason, rather than rounding up to "all caught".
## Cold re-read. Two things before this can merge, one of them mine to decide.
**Re-measured here:** offline **83 passed, 3 skipped, 0 failed**. Secret scan of the diff is clean — the only credential-shaped strings are the deliberate `AKIAFAKETESTKEY` fakes. Public-repo constraint holds.
The publish work is good. The index filename list is right and, better, *reasoned*: matching `index-v1.json(.asc)`, `index-v2.json`, `entry.json` and `entry.jar` **only at the top level**, with a note that a nested `diff/<ts>.json` is not "the index" and is only ever fetched by a client that already holds the previous one. That is the kind of distinction that would have been got wrong by pattern-matching on `index*`.
Also right: the credential check makes a **real** `head-bucket` call rather than testing that variables are non-empty, which is exactly the "set but not usable" case bit asked for.
## 1. The test hook has to go
```bash
run_test_hook() {
local hook_value="${!hook_var:-}"
[ -n "$hook_value" ] && eval "$hook_value"
}
```
This `eval`s an environment variable, in a script that runs **with the release signing key and S3 credentials in its environment**, in a **public** repository. The comment says "never set by a real caller", and that is true today — but it is a documented, discoverable code path that turns any env-var influence into arbitrary code execution at exactly the worst moment.
I do not think the threat model makes it *likely*. I think it is unnecessary, which is the better argument.
**The alternative is a feature rather than a seam:** split the two passes into separately invokable steps — `--files-only` and `--index-only`. Then ordering is testable by construction (run pass 1, assert the bucket state, run pass 2) with no hook, no `eval`, and no test-only branch in production code.
It is also independently useful: an interrupted publish can be resumed at the index step without re-uploading every APK, and a caller who wants to stage now and commit later can.
## 2. The granularity is still open, and this implements minutes
`next_version_code.py` uses minutes since 2026-01-01. That was the state when you started, but bit and I have since worked through it on weblib-viewer#15 and the requirement changed: bit wants **every build kept**, which makes collisions something to render impossible rather than resolve.
My proposal there is **5-second granularity** — 340 years of headroom, and a collision requires two builds starting within the same 5 seconds. **bit has not answered yet**, so this is not a change to make now; it is a one-constant change when they do.
Worth noting the clamp survives either way, but its *justification* changes: not concurrency any more, but a clock stepping backwards and rebuilding an older commit. The comments should say that when the constant lands.
## Well handled, and worth keeping
- **`versionCode` read from `packages.<appId>.versions.*.manifest.versionCode`, verified against fdroidserver's own `index.py`** rather than assumed from the JSON's shape.
- The local HEAD-only S3 stub, which makes the credential and collision checks testable **offline**. That is the difference between a check that is tested and one that is only exercised when someone has credentials.
- Finding, while writing the live test, that `publish.sh` exports the Garage checksum variables only into its own child — so the test's own direct write hit the exact `InvalidRequest` from #3. Fixed in the test and commented. That is the bug biting from a new direction, and catching it in the test rather than in production is the whole point of having a live test.
- Honest reporting that one of four collision assertions stayed green for an unrelated reason, rather than rounding up to "all caught".
bit's review on PR #7 (comment on the PR, appended to this reasoning):
run_test_hook() eval'd an environment variable in publish.sh, which runs
with the release signing key and S3 credentials in its environment, in a
public repo. "Never set by a real caller" was true, but it was still a
documented, discoverable path from any env-var influence to arbitrary code
execution at the worst possible moment -- unnecessary, which is the
stronger reason to remove it than any claim about likelihood.
The replacement is a feature rather than a seam: --files-only and
--index-only split the two passes into separately invokable steps. With
neither flag the default is unchanged (both passes, files first). This
makes the ordering guarantee testable by construction -- run --files-only,
assert the bucket state, run --index-only, assert again -- with no hook, no
eval, and no test-only branch in production code. It is also independently
useful: an interrupted publish can resume at the index step without
re-uploading every APK, and a caller can stage files now and publish the
index later. The APK-existence check is skipped entirely for a plain
--index-only invocation, since that pass can never upload a *.apk.
Re-tested end to end: offline 91 passed / 4 skipped / 0 failed (up from 83/
3/0, mostly new flag-construction and stub assertions); live against
fdroid-weblib-dev 104 passed / 0 skipped / 0 failed, bucket verified empty
before and after. Added a live fault-injection test that --index-only alone
against a prefix --files-only never touched really does publish an index
naming an absent file -- the exact state the ordering guarantee exists to
prevent -- contrasted with the default path, which never reaches it.
Offline, removing the run_files/run_index gating (simulating the flags
being ignored) was confirmed to fail 5 of the new assertions before being
reverted.
Left the versionCode granularity as minutes, per instruction: that is being
settled separately on weblib-viewer#15 and is a one-constant change for
whenever it lands, not something to pre-empt here.
Co-authored-by: bit <bit@das-labor.org>
Addressed: run_test_hook removed, replaced with --files-only/--index-only
Pushed ccb8cb6. publish.sh no longer evals anything -- grep -c "eval " publish.sh is 0. The two passes are now separately invokable flags, mutually exclusive, default unchanged (both, files then index).
The ordering guarantee is now tested by construction: run --files-only, check the bucket for real, run --index-only, check again -- two genuinely separate invocations, no synchronization point inside publish.sh at all.
Fault injection requested: added a live test that runs --index-onlyalone against a prefix --files-only never touched, and asserts it really does publish an index naming an absent file -- the exact "index naming files that 404" state the whole guarantee exists to prevent. Contrasted with the default path (neither flag, one call), which is checked to never reach that state; this holds by construction, since the default's first phase runs the identical cmd_files array --files-only runs, so there is no code path from the default to "index present, file absent" without invoking --index-only alone, which is opt-in.
Also offline: reverted the run_files/run_index gating (simulating the flags being silently ignored) and confirmed 5 of the new assertions fail, then restored.
Re-measured:
Offline: 91 passed, 4 skipped, 0 failed (up from 83/3/0 -- new flag-construction and stub assertions).
Live against fdroid-weblib-dev: 104 passed, 0 skipped, 0 failed. Bucket verified empty before and after.
Kept everything else as-is per your note: top-level-only index filename matching (and the diff/<ts>.json reasoning), the real head-bucket credential check, the APK-existence refusal, the offline S3 stub, and the checksum-export fix in the test. Left next_version_code.py's granularity as minutes, since that's a separate one-constant change pending weblib-viewer#15.
## Addressed: `run_test_hook` removed, replaced with `--files-only`/`--index-only`
Pushed `ccb8cb6`. `publish.sh` no longer `eval`s anything -- `grep -c "eval " publish.sh` is 0. The two passes are now separately invokable flags, mutually exclusive, default unchanged (both, files then index).
The ordering guarantee is now tested by construction: run `--files-only`, check the bucket for real, run `--index-only`, check again -- two genuinely separate invocations, no synchronization point inside `publish.sh` at all.
**Fault injection requested:** added a live test that runs `--index-only` **alone** against a prefix `--files-only` never touched, and asserts it really does publish an index naming an absent file -- the exact "index naming files that 404" state the whole guarantee exists to prevent. Contrasted with the default path (neither flag, one call), which is checked to never reach that state; this holds by construction, since the default's first phase runs the identical `cmd_files` array `--files-only` runs, so there is no code path from the default to "index present, file absent" without invoking `--index-only` alone, which is opt-in.
Also offline: reverted the `run_files`/`run_index` gating (simulating the flags being silently ignored) and confirmed 5 of the new assertions fail, then restored.
**Re-measured:**
- Offline: **91 passed, 4 skipped, 0 failed** (up from 83/3/0 -- new flag-construction and stub assertions).
- Live against `fdroid-weblib-dev`: **104 passed, 0 skipped, 0 failed**. Bucket verified empty before and after.
Kept everything else as-is per your note: top-level-only index filename matching (and the `diff/<ts>.json` reasoning), the real `head-bucket` credential check, the APK-existence refusal, the offline S3 stub, and the checksum-export fix in the test. Left `next_version_code.py`'s granularity as minutes, since that's a separate one-constant change pending weblib-viewer#15.
The eval is gone — verified directly rather than from the report: no eval in any production script, no TEST_HOOK reference anywhere in the repo. Offline 91 passed, 4 skipped, 0 failed.
The replacement is better than the hook it removes, and the comment says why in the right register:
This is not a test seam -- it is real, useful on its own
That is the distinction that matters. --files-only / --index-only make the ordering testable by running the two passes separately rather than by reaching into the middle of one, and they are independently useful for resuming an interrupted publish. Skipping the APK-existence check on a plain --index-only is right too, since that pass cannot upload one.
The fault injection is the part I would have asked for if it had not been done: invoking --index-only alone really does produce an index naming a file that 404s — the exact dangerous state this issue exists to prevent — with a check that the default path never reaches it. A test that only proves the good path works would not have shown the failure mode is real.
Merging with a known-pending constant, deliberately
next_version_code.py uses minutes, and that is still open on weblib-viewer#15 — bit wants every build kept, which makes 5-second granularity the likely answer.
Merging anyway because nothing calls next-version-code.sh yet: no workflow, no Makefile. So the constant has no operational effect today, and blocking a finished publish-ordering change on an open question about one number would be the wrong trade. When bit decides, it is one constant and its tests.
#5 is fully delivered and I am closing it. #6 stays open until the granularity lands, since its rule is not final.
## Re-read of the fix. Merging.
**The `eval` is gone** — verified directly rather than from the report: no `eval` in any production script, no `TEST_HOOK` reference anywhere in the repo. Offline **91 passed, 4 skipped, 0 failed**.
The replacement is better than the hook it removes, and the comment says why in the right register:
> This is not a test seam -- it is real, useful on its own
That is the distinction that matters. `--files-only` / `--index-only` make the ordering testable by *running the two passes separately* rather than by reaching into the middle of one, and they are independently useful for resuming an interrupted publish. Skipping the APK-existence check on a plain `--index-only` is right too, since that pass cannot upload one.
The fault injection is the part I would have asked for if it had not been done: invoking `--index-only` alone really does produce an index naming a file that 404s — **the exact dangerous state this issue exists to prevent** — with a check that the default path never reaches it. A test that only proves the good path works would not have shown the failure mode is real.
## Merging with a known-pending constant, deliberately
`next_version_code.py` uses **minutes**, and that is still open on weblib-viewer#15 — bit wants every build kept, which makes 5-second granularity the likely answer.
Merging anyway because **nothing calls `next-version-code.sh` yet**: no workflow, no Makefile. So the constant has no operational effect today, and blocking a finished publish-ordering change on an open question about one number would be the wrong trade. When bit decides, it is one constant and its tests.
`#5` is fully delivered and I am closing it. **`#6` stays open** until the granularity lands, since its rule is not final.
claude
merged commit fc2735624e into main2026-09-09 08:19:55 +00:00
claude
deleted branch publish-ordering-and-version-code2026-09-09 08:19:55 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
bit
Problem
publish.shuploaded a whole repo tree with a singleaws s3 sync, givingno control over upload order. An F-Droid repo is a signed index plus the
files it names, and an interrupted publish could land the index before an
APK it references -- breaking the repo for every client until the upload
finished or was rolled back (#5). Credentials were only checked for being
set, not for being usable, which is exactly how a partial upload can
start in the first place.
Separately, nothing enforced one shared rule for computing a monotonic
versionCodeacross CI and a manual build (#6). While implementing that,bit pointed out (appended to #6 mid-implementation) that the clamp
next-version-code.shuses is monotonic against observed state, notmonotonic by construction: two jobs starting in the same minute can both
read the same published value and race a whole build to publish two
different APKs under the same
versionCode.What changed
publish.shuploads in two passes, unconditionally: every non-indexfile first, then
index-v1.json(.asc)/index-v2.json/entry.json/entry.jar(top-level names only) last. An interruption anywhere in pass1 leaves the repo describing the old state a little longer; it can never
leave an index naming files that 404.
(
head-bucket) before either pass, not just checked for being non-empty.next-version-code.sh(+ stdlib-onlynext_version_code.py): oneshared rule --
candidate = minutes since 2026-01-01T00:00:00Z, clamped tostrictly exceed whatever is already published for that
applicationId--called by both CI and a manual build.
publish.shrefuses to overwrite an already-existing APK object,checked before pass 1. This can't prevent the wasted build behind a
same-minute race; it converts the collision from a silent, permanent
wrong state (two different APKs claiming one
versionCode) into a loudfailure at the last moment the information is reliable, as required
alongside the clamp.
See the expanded "One versionCode rule" and "Publish ordering and safety"
sections in the README for the full reasoning, including why "monotonic by
construction" was wrong as previously written and is not claimed again.
Testing
Offline: 83 assertions, 0 failed, 0 skipped, across 7 suites (
bash run_tests.sh), including two new ones:test_next_version_code.py-- pure stdlib, no nix: no prior build, lower/equal/higher prior (clock-went-backwards), several versions, a version
with no
versionCode, an application id missing entirely (not an error),and unreadable/malformed indexes (fail loudly, nothing on stdout).
test_next_version_code.sh-- the real CLI throughwith-nixpkgs.sh:argument count, a local index file end to end, a missing application id,
a missing/malformed file, and an unreachable URL (closed port).
test_publish.shgained: dry-run assertions that the two commands areprinted in order (files, then index) with the right
--exclude/--include; a real invocation against a closed port proving the failurehappens during credential verification, before any upload; and a new
local HEAD-only S3 stub (
test_s3_stub.py, pure stdlib) proving thecredential check is a genuine authenticated call (a wrong bucket is
rejected) and that an already-published APK is refused before any upload,
with a negative control proving a non-colliding APK does not trip it.
Live, against
fdroid-weblib-dev: 34 assertions, 0 failed (the 27offline ones for that file, plus 7 that only run against a real bucket):
a real publish, an already-published APK refused with its bytes unchanged
afterwards, and -- the hard part, since the offline suite structurally
cannot see the order two real uploads land in -- the index proven genuinely
absent from the bucket while the file object was already there, observed
from inside
publish.sh's own process via a same-process synchronizationseam (
PUBLISH_SH_TEST_HOOK_BETWEEN_PASSES) rather than a poll racing abackground upload, which is exactly the flakiness #5 warned against.
Bucket left empty after every run (verified by listing it before and after).
Every change here was also fault-injected and reverted to confirm the new
tests actually catch it: removing the two-pass filtering (5 failures),
removing the credential check (4 failures), and removing the APK-collision
check (3 of 4 related assertions failing, the 4th coincidentally still
passing for an unrelated reason -- noted in the test comments). Full detail
in the session report, not reproduced here.
What I did not do
concurrency:group to serialise publishes -- #6 explicitlyasked not to rely on that, since whether Gitea 1.25.2 honours it is
unverified.
genuine concurrent collision" in the APK-existence check -- both look
identical at the object level without hashing every object, which was
judged not worth the complexity here. Documented as an accepted cost in
publish.sh.Cold re-read. Two things before this can merge, one of them mine to decide.
Re-measured here: offline 83 passed, 3 skipped, 0 failed. Secret scan of the diff is clean — the only credential-shaped strings are the deliberate
AKIAFAKETESTKEYfakes. Public-repo constraint holds.The publish work is good. The index filename list is right and, better, reasoned: matching
index-v1.json(.asc),index-v2.json,entry.jsonandentry.jaronly at the top level, with a note that a nesteddiff/<ts>.jsonis not "the index" and is only ever fetched by a client that already holds the previous one. That is the kind of distinction that would have been got wrong by pattern-matching onindex*.Also right: the credential check makes a real
head-bucketcall rather than testing that variables are non-empty, which is exactly the "set but not usable" case bit asked for.1. The test hook has to go
This
evals an environment variable, in a script that runs with the release signing key and S3 credentials in its environment, in a public repository. The comment says "never set by a real caller", and that is true today — but it is a documented, discoverable code path that turns any env-var influence into arbitrary code execution at exactly the worst moment.I do not think the threat model makes it likely. I think it is unnecessary, which is the better argument.
The alternative is a feature rather than a seam: split the two passes into separately invokable steps —
--files-onlyand--index-only. Then ordering is testable by construction (run pass 1, assert the bucket state, run pass 2) with no hook, noeval, and no test-only branch in production code.It is also independently useful: an interrupted publish can be resumed at the index step without re-uploading every APK, and a caller who wants to stage now and commit later can.
2. The granularity is still open, and this implements minutes
next_version_code.pyuses minutes since 2026-01-01. That was the state when you started, but bit and I have since worked through it on weblib-viewer#15 and the requirement changed: bit wants every build kept, which makes collisions something to render impossible rather than resolve.My proposal there is 5-second granularity — 340 years of headroom, and a collision requires two builds starting within the same 5 seconds. bit has not answered yet, so this is not a change to make now; it is a one-constant change when they do.
Worth noting the clamp survives either way, but its justification changes: not concurrency any more, but a clock stepping backwards and rebuilding an older commit. The comments should say that when the constant lands.
Well handled, and worth keeping
versionCoderead frompackages.<appId>.versions.*.manifest.versionCode, verified against fdroidserver's ownindex.pyrather than assumed from the JSON's shape.publish.shexports the Garage checksum variables only into its own child — so the test's own direct write hit the exactInvalidRequestfrom #3. Fixed in the test and commented. That is the bug biting from a new direction, and catching it in the test rather than in production is the whole point of having a live test.Addressed:
run_test_hookremoved, replaced with--files-only/--index-onlyPushed
ccb8cb6.publish.shno longerevals anything --grep -c "eval " publish.shis 0. The two passes are now separately invokable flags, mutually exclusive, default unchanged (both, files then index).The ordering guarantee is now tested by construction: run
--files-only, check the bucket for real, run--index-only, check again -- two genuinely separate invocations, no synchronization point insidepublish.shat all.Fault injection requested: added a live test that runs
--index-onlyalone against a prefix--files-onlynever touched, and asserts it really does publish an index naming an absent file -- the exact "index naming files that 404" state the whole guarantee exists to prevent. Contrasted with the default path (neither flag, one call), which is checked to never reach that state; this holds by construction, since the default's first phase runs the identicalcmd_filesarray--files-onlyruns, so there is no code path from the default to "index present, file absent" without invoking--index-onlyalone, which is opt-in.Also offline: reverted the
run_files/run_indexgating (simulating the flags being silently ignored) and confirmed 5 of the new assertions fail, then restored.Re-measured:
fdroid-weblib-dev: 104 passed, 0 skipped, 0 failed. Bucket verified empty before and after.Kept everything else as-is per your note: top-level-only index filename matching (and the
diff/<ts>.jsonreasoning), the realhead-bucketcredential check, the APK-existence refusal, the offline S3 stub, and the checksum-export fix in the test. Leftnext_version_code.py's granularity as minutes, since that's a separate one-constant change pending weblib-viewer#15.Re-read of the fix. Merging.
The
evalis gone — verified directly rather than from the report: noevalin any production script, noTEST_HOOKreference anywhere in the repo. Offline 91 passed, 4 skipped, 0 failed.The replacement is better than the hook it removes, and the comment says why in the right register:
That is the distinction that matters.
--files-only/--index-onlymake the ordering testable by running the two passes separately rather than by reaching into the middle of one, and they are independently useful for resuming an interrupted publish. Skipping the APK-existence check on a plain--index-onlyis right too, since that pass cannot upload one.The fault injection is the part I would have asked for if it had not been done: invoking
--index-onlyalone really does produce an index naming a file that 404s — the exact dangerous state this issue exists to prevent — with a check that the default path never reaches it. A test that only proves the good path works would not have shown the failure mode is real.Merging with a known-pending constant, deliberately
next_version_code.pyuses minutes, and that is still open on weblib-viewer#15 — bit wants every build kept, which makes 5-second granularity the likely answer.Merging anyway because nothing calls
next-version-code.shyet: no workflow, no Makefile. So the constant has no operational effect today, and blocking a finished publish-ordering change on an open question about one number would be the wrong trade. When bit decides, it is one constant and its tests.#5is fully delivered and I am closing it.#6stays open until the granularity lands, since its rule is not final.