Releases: POST /releases/sync for the upload script - #83
Merged
Merged
Conversation
The scheduled tick found a new version at most 30 minutes plus the settle window after upload. The upload script knows when its last object is written, so it can trigger the sync itself. One runner now owns the in-progress flag and serves both the timer and the endpoint. The endpoint is guarded by RELEASE_SYNC_TOKEN as a bearer token, compared in constant time, and is not registered without it. It runs with the settle window off and answers with the per-outcome counts, or 409 while a run is in progress.
adamshiervani
marked this pull request as ready for review
September 19, 2026 13:07
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19e90278e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| */ | ||
| export function Sync(runner: ReleaseSyncRunner) { | ||
| return async (req: Request, res: Response) => { | ||
| const stats = await runner({ uploadSettleMs: 0 }); |
There was a problem hiding this comment.
When another firmware version or type is still being uploaded, a POST made after an independent upload completes performs a bucket-wide scan with the settle window disabled. It can therefore persist the other upload's partial SKU set or stale hash, and syncReleases deliberately never updates that row after the remaining objects arrive. Scope the bypass to the specific completed upload, or retain settling for all unrelated versions.
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
Addressed in #84: the body names the version the caller finished, and only that version skips the settle window. Every other version keeps it.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The authentication parser rejects valid case-insensitive Bearer scheme variants.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Adds a token-protected endpoint for immediately synchronizing uploaded releases while retaining scheduled synchronization.
Changes:
- Refactors synchronization into a shared single-runner abstraction.
- Adds bearer-token authentication and
POST /releases/sync. - Adds conflict handling, configuration, and tests.
| File | Description |
|---|---|
.env.example |
Documents the synchronization token. |
src/auth.ts |
Adds static bearer-token middleware. |
src/errors.ts |
Adds HTTP 409 support. |
src/index.ts |
Registers and schedules the shared runner. |
src/release-sync.ts |
Extracts runner and scheduler responsibilities. |
src/releases.ts |
Implements the synchronization handler. |
test/auth.test.ts |
Tests bearer-token authentication. |
test/sync-releases.test.ts |
Tests scheduling and endpoint behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| export const bearerToken = (expected: string) => { | ||
| const expectedDigest = sha256(expected); | ||
| return (req: Request, res: Response, next: NextFunction) => { | ||
| const presented = req.headers.authorization?.match(/^Bearer (.+)$/)?.[1]; |
Contributor
Author
There was a problem hiding this comment.
Addressed in #85: the scheme is matched case-insensitively with one or more spaces, and the token is still compared exactly.
adamshiervani
added a commit
that referenced
this pull request
Sep 19, 2026
* fix(releases): serve staged releases before any release reaches 100% (#78) A prefix with no release at 100% made the default lookup throw a 500 before eligibility was checked, so no device got the staged build. That is every device for a new prefix until its first release is fully rolled out, and every JetKVM device the day no app or system row sits at 100%. The default lookup now returns null when nothing is at 100%. A device inside the rollout bucket gets the staged release as before; a device outside it gets a 404 saying no release is rolled out yet for its SKU, instead of a 500. Found by Bugbot on the release PR (#77). * Releases: sync new R2 releases from the API every 30 minutes (#81) * feat(releases): sync new R2 releases from the API every 30 minutes The release sync only ran when an operator invoked scripts/sync-releases.ts by hand, so a firmware upload sat in R2 until someone remembered to run it. The API process now runs the same sync on start and every 30 minutes, registering each new stable version at the default 10% rollout. The non-interactive core (bucket listing, artifact collection, DB insert) moves from the script into src/release-sync.ts so the API can import it; the script keeps the GPG check and the confirmation prompt and passes them in as the release decider. Both entry points share one R2 client via src/s3.ts. Scheduled runs skip a tick while the previous run is still going, log and survive a failed run, and treat a unique-constraint hit on insert as "already synced" so several API instances can race without an error. The existence check now precedes the R2 artifact walk, so a tick over an already-synced bucket costs one list call per prefix plus one DB lookup per version. * fix(release-sync): defer versions whose upload is still settling A scheduled tick can land while a version is still being uploaded and register it with a partial SKU set or a hash that changes afterwards. Sync never rewrites a row, so that snapshot would be permanent. Before collecting artifacts, list every object under the version folder and skip the version when the newest object changed in the last 10 minutes. The window is shorter than the schedule interval, so a real upload costs at most one extra tick. * refactor(release-sync): share R2 config, paginate via SDK, probe SKUs concurrently * src/s3.ts now exports bucketName and baseUrl next to the client, so the request handlers, the scheduler and the script read R2 config from one place instead of three. * The upload settle window moves into SyncConfig.uploadSettleMs. The scheduler sets it; the one-shot operator script leaves it unset because an operator sees the artifact list before confirming and has no next run. * newestUploadTime uses the SDK's paginateListObjectsV2 instead of a hand-rolled continuation loop. * collectReleaseArtifacts probes SKUs concurrently and folds the results in SKU order, so artifact order and the primary artifact are unchanged. * Tests drop empty per-prefix listing stubs that the file-level default already covers. * fix(release-sync): take the settle listing after the artifact scan Listing the version folder before the scan left a gap: an upload that started between the listing and the scan passed the check and was captured partially. Listing after the scan closes it, because any upload active during the scan leaves an object newer than the window and the snapshot is discarded instead of registered. * fix(release-sync): paginate the version listing ListObjectsV2 returns at most 1,000 common prefixes per page. Once a release prefix grows past that, versions on later pages were never seen on any tick. listStableVersions now walks every page with the SDK paginator, as newestUploadTime already does, and the mock in the sync tests can serve a truncated listing. * Release sync: a unique violation is only a race when the row exists (#82) * fix(release-sync): only treat a unique violation as a race when the row exists Sync caught every P2002 from the release insert as "created concurrently elsewhere". On staging the id sequences were behind the rows after a data import, so each insert failed on the primary key, was logged as a race, and left nothing in the table. After a unique violation, createRelease now looks the (version, type) row up. Present means another instance registered it first; absent means the insert really failed, and the error is rethrown with the type and version in its message so the scheduled run log names the release. * fix(release-sync): name the release in every per-version failure Wrapping only the insert error left the row lookup, and the S3 scan before it, free to escape without the type and version. syncReleases now wraps whatever createRelease throws for a version. * feat(releases): POST /releases/sync for the upload script (#83) The scheduled tick found a new version at most 30 minutes plus the settle window after upload. The upload script knows when its last object is written, so it can trigger the sync itself. One runner now owns the in-progress flag and serves both the timer and the endpoint. The endpoint is guarded by RELEASE_SYNC_TOKEN as a bearer token, compared in constant time, and is not registered without it. It runs with the settle window off and answers with the per-outcome counts, or 409 while a run is in progress. * fix(auth): accept the bearer scheme in any case (#85) Scheme names are case-insensitive (RFC 9110). The token is still compared exactly. * fix(releases): scope the sync endpoint's settle bypass to one version (#84) The endpoint turned the settle window off for every version in the bucket. A call made while a different upload was still running registered that upload half done, and sync never revisits a row. The body now names the version the caller finished, and only that version skips the window. Without a body the call is a normal tick.
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.
A new version reached the database only when the 30 minute tick found it, and then only after a 10 minute settle window. The upload script knows when its last object is written, so it can trigger the sync itself. This adds
POST /releases/syncfor that call. The tick stays as the safety net.The endpoint
No body, no query. The run is synchronous, about two seconds on staging, so the caller can fail loudly on a bad status. Without
RELEASE_SYNC_TOKENthe route is not registered and Express answers 404.One runner, two triggers
sequenceDiagram participant Up as upload script participant API participant Run as runner participant R2 participant DB Up->>R2: write artifacts + .sha256 Up->>API: POST /releases/sync (bearer) API->>Run: run({ uploadSettleMs: 0 }) alt already running Run-->>API: "busy" API-->>Up: 409 else Run->>R2: list stable versions Run->>DB: create missing releases @10% Run-->>API: counts API-->>Up: 200 counts end Note over API,Run: the 30 min tick calls run() with the settle windowSettle window
The scheduled tick defers a version whose newest object changed in the last 10 minutes, because it cannot know whether an upload is finished. The endpoint turns that off. The contract is that the caller calls it after its last write.
uploadSettleMs: 0is the same mode the interactive script already uses.Busy
The running flag is per process. Several API instances can still start a run each; the unique constraint on
(version, type)settles that race as before. A 409 only says this instance is mid-run, and the caller can retry.Still to do outside this repo
Add the call to the end of the firmware upload script, once per environment it uploads for, and set
RELEASE_SYNC_TOKENon staging and production.Note
Medium Risk
Introduces a token-gated endpoint that writes release rows to the database; misconfiguration or token leakage could allow unauthorized sync triggers, though rollout logic is unchanged.
Overview
Adds
POST /releases/syncso the firmware upload script can register new R2 releases immediately after upload, instead of waiting for the 30‑minute scheduler and 10‑minute settle window. The route is registered only whenRELEASE_SYNC_TOKENis set and is guarded by newbearerTokenmiddleware (constant-time compare).Release sync is refactored into a shared
createReleaseSyncRunner: one in-process run at a time for both the timer and HTTP trigger. The handler runs sync withuploadSettleMs: 0, returns per-outcome JSON counts on success, and409 Conflictwhen a sync is already running.ConflictErroris added for that case; tests cover bearer auth, scheduling, and the sync handler.Reviewed by Cursor Bugbot for commit 19e9027. Bugbot is set up for automated code reviews on this repo. Configure here.