Skip to content

Refuse a load-harness summary when the deployment moved under the run - #6197

Open
habdelra wants to merge 5 commits into
mainfrom
cs-13033-staging-redeploys-mid-measurement-silently-invalidating
Open

habdelra wants to merge 5 commits into
mainfrom
cs-13033-staging-redeploys-mid-measurement-silently-invalidating

Conversation

@habdelra

@habdelra habdelra commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

A run's numbers describe one build of one fleet. Staging moves on its own
schedule, and a window long enough to measure anything is long enough for a
deploy to land inside it — during one 55-minute session the staging host bundle
changed twice. A window that straddles a deploy holds two runs averaged
together, and a moved number cannot be told from a moved deployment. Until now
the only defence was remembering to check the task-definition revision and the
bundle hash either side of every run, which catches the problem after the
measurement time has already been spent.

run-load.ts now reads the deployment before and after each run and refuses to
print a summary if it moved. Both readings come from the host app's boot
document at <realm server>/_standby, over plain HTTP with no AWS session, so
the check works against every target the harness points at:

  • the host build — the entry bundle the document loads
    (assets/main-<hash>.js) and the host's own build version from its config
    meta (0.0.0+<sha>);
  • which replicas answered — the container id in
    X-ECS-Container-Metadata-URI-v4, so a fleet that changed is visible even
    when the task definition did not.

Two outcomes end a run early rather than late. A fleet already serving two
host builds
is refused before authentication — that costs a probe rather than
the window. A build that moved, or a fleet that gained or lost a replica
by the close, replaces the summary with what moved: an arrival served part of
the window cold, a departure means the rest of the fleet carried a different
share of the load partway through, and a replaced task is both.

Two outcomes leave the run intact and say what is not known about it, because
neither is evidence that anything moved. Nothing naming a build at the start
reads build: not pinned — a run cannot be refused for failing a check it
never passed. No boot document coming back at the close reads NOT CONFIRMED,
and the summary still prints: silence, or a fleet answering only errors, says
the pin is unknown, and the likeliest target to go quiet at the close is the
one the harness has just spent an hour saturating. In the same spirit, an
answer that identifies its replica but carries no usable document keeps the
replica and drops only the build, so a transient 502 is not read as a task
that left — and a build is taken only from a document that named a bundle or a
version, so a 200 carrying an error page cannot stand beside the real build as
a second one.

Reading a fleet is a sampling problem

The comparison refuses a run on what two samples differ by, so a sample that
misses a replica invents a deploy. Three properties carry that decision, each
measured rather than assumed:

  • Probes within a wave are concurrent.
  • Every probe opens its own connection. Concurrency alone widens the sample
    only within a wave — the pool's sockets are free again by the time the next
    wave is issued, and undici prefers a free socket to a new one. Measured
    against a server reporting the socket each request arrived on: three waves of
    four reach 4, 5, 5 distinct sockets with keep-alive and 4, 8, 12 with
    Connection: close. Without it a fleet of eight was refused with nothing
    deployed, because the run's own traffic widened the pool and the closing
    reading reached replicas the opening one structurally could not.
  • The closing reading keeps probing while any replica the opening one saw has
    yet to answer
    , which is what lets a replica be called departed rather than
    merely unseen.

The loops that generate the load recheck the clock after the awaits that can
outlast it — priming opens a connection per query, and a forwarded call is a
round trip of its own — so no batch of searches or write is dispatched into a
window the closing pin no longer covers.

Two mechanisms on the server side decided the rest, both verified against
staging: each replica caches the boot document for the life of its process,
so two replicas can serve two different host builds at once and a browser gets
whichever answers; and a reading therefore keeps its builds apart from the
replicas that served them
, or a target that identifies no replicas files every
response under one key and the builds overwrite one another until only the last
probe's survives.

On the task-definition revision

The ticket asks for the realm-server task-def revision at both ends. This reads
what the fleet serves instead, because a revision change always replaces
tasks and so always shows up here as a changed fleet — and because reaching ECS
would put an AWS session in the path of a driver that deliberately has no
dependencies and runs from CloudShell or an ECS task. The rolloutState check
before an A/B is unchanged and still the operator's; the README and the skill
say so explicitly.

Verification

Unit coverage in tests/load-harness-test.ts — 30 tests, all pure, no pg and
no fixture ports: bundle and version parsing off a document shaped like the
served one, the entry distinguished from both an inline script and the preloads
that precede it, wave concurrency, the connection and Accept headers every
probe carries, wave convergence, the expectation-driven closing probe, the
probe cap, error pages and unreachable targets, straddle detection with and
without replica identity, an erroring replica keeping its identity and losing only its
document, and each drift rule including a header-less deployment reporting its
build moving and nothing else.

Every rule was confirmed to fail when the behaviour it names is removed:
dropping Connection: close, filing builds under the replica key, deleting the
departure arm, dropping the expectation from the closing probe, routing
close-side silence back into a refusal, removing the guard on an empty opening
reading, discarding an erroring replica's id, confirming a pin on any answer
rather than on a document, and recording a build from a document that named
none, each reddened exactly the tests that assert them and nothing else.

End to end against staging the probe reads the live fleet — two replicas, one
build, no straddle, no drift. Across today's work it read three different host
builds and three different pairs of container ids from the same target, which
is the failure this PR exists to catch: a run spanning any two of those moments
would now be refused instead of summarised.

pnpm lint in packages/realm-server passes lint:js; lint:types reports
four pre-existing errors in @cardstack/boxel-ui sources that a worktree
without a boxel-ui build always shows, none in the files this PR touches. The
suite also passes through the real runner (node tests/index.ts with
TEST_FILES/TEST_MODULES narrowed), so the new cases load under the require
path CI uses: 102 tests, 0 failures.

🤖 Generated with Claude Code

habdelra and others added 2 commits September 17, 2026 23:28
A run's numbers describe one build of one fleet, and a window long enough
to measure anything is long enough for a deploy to land inside it. The
driver now reads the host app's boot document before and after a run: the
entry bundle and the host build version it names, and the container id of
each replica that answered. If either build identifier moved, if a replica
answered the close that had not answered the start, or if nothing answered
the close, the summary is replaced by what moved.

A fleet already serving two host builds is refused before authentication.
Each replica fetches that document once and caches it for the life of its
process, so replicas started either side of a host deploy serve different
builds at the same moment; catching it there costs a probe rather than the
measurement window.

Probes go out in concurrent waves until a wave meets nobody new, because
sequential probes reuse one keep-alive socket and so reach one replica
however often they are repeated.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 18, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-18T04:20:20.280747Z c8e239a Manual request
ℹ️ 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" or "@codex security review".

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 chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c0ed561933

ℹ️ 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".

Comment thread packages/realm-server/scripts/load-harness/lib/deploy-pin.ts Outdated
Comment thread packages/realm-server/scripts/load-harness/lib/deploy-pin.ts Outdated

@habdelra habdelra left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] This review went after the new module's reading of the fleet — I stood up a local round-robin fleet that reports which socket each request arrived on and ran readFleet at it, and checked the HTML parsing against what serve-index.ts actually emits. The deployed-side claims (what staging served, and that it redeployed between two probes) I took on trust; I have no session against that environment.

The refusal rule is sound, but the reading it acts on is incomplete, and that inverts the trade this PR is built on: on a fleet larger than one wave's worth of connections it refuses runs in which nothing deployed. Two things to fix before merge — a one-line probe change, and a guard for an opening reading that answered nothing. The rest are one decision and some test gaps.

On the bot's two findings:

  • P1, vanished replicas. Don't add that arm yet. Measured on a fleet of 8 with a 6 s idle gap and no deploy, today's sampling produces three spurious arrivals and three spurious departures in the same pair of readings — a departure arm on top of that refuses more runs without catching more deploys. It becomes safe once a reading is complete; detail in the readFleet thread.
  • P2, builds collapsed under one key. Confirmed, with the measurement, in the thread on replicas.set. The comment on UNIDENTIFIED_REPLICA claims more than the code delivers, so that half needs fixing whichever way you go.

Recommendations, in order:

  1. Probe each replica on its own connection — readFleet thread, with the measured one-line change.
  2. Refuse up front when the opening reading answered nothing; otherwise the "build moved" line prints a blank where the starting build goes — run-load.ts opening-block thread.
  3. Decide whether close-side silence refuses or reports unpinned — fleetDrift thread.
  4. Two of the new tests pass with the rule they name deleted — the two test-file threads.
  5. Bound the interrupt path — SIGINT thread.
  6. Drop the dated clause from the module header — thread on the X-ECS-Container-Metadata-URI-v4 paragraph.

One thing I checked and won't ask you to change: the opening probe is correctly sequenced after measureConnectionSetup, so the cold sample is still cold.

Comment on lines +150 to +171
for (let wave = 0; wave < maxWaves; wave++) {
let known = replicas.size;
let results = await Promise.all(
Array.from({ length: waveSize }, () =>
probeOnce(url, fetchImpl, timeoutMs),
),
);
probes += waveSize;
for (let result of results) {
if (!result) {
continue;
}
responses++;
replicas.set(result.replicaId ?? UNIDENTIFIED_REPLICA, result.build);
}
// Stop once the fan-out stops discovering. The first wave is exempt so a
// target that answered nothing gets a second chance rather than being
// reported as unreachable on one unlucky moment.
if (replicas.size === known && wave > 0) {
break;
}
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] The wave loop doesn't widen the connection sample, so a reading stops growing long before it has seen the fleet — and since the opening reading is taken before the run and the closing one after, the two aren't comparable. On a fleet of six this refuses a run in which nothing deployed.

The keep-alive reuse the header comment names between probes also operates between waves: wave 1 is issued in the microtask after Promise.all settles, undici's clients are free by then, and it lands on the sockets wave 0 opened. Against a local server that reports which socket each request arrived on, three waves at an 8-replica round-robin fleet read:

sockets seen: [0 1 2 3] [0 1 2 4] [0 1 2 3]   ->  5 of 8 replicas, 12 probes

So maxWaves buys about one extra connection, not waveSize more. The run's own traffic then widens undici's pool, so the closing probe reaches replicas the opening one never could:

fleet of 6, nothing deployed: opening 5, closing 6  -> "1 replica answered the closing probe…"
fleet of 8, nothing deployed: opening 5, closing 8  -> "3 replicas answered the closing probe…"

Forcing each probe onto its own connection fixes it and keeps this directory dependency-free — in BOOT_HEADERS:

const BOOT_HEADERS = { Accept: 'text/html', Connection: 'close' };

With that header the same scenarios read 8/8 and 16/16 discovered at both ends and fleetDrift clean, repeatably.

Two things to fold in while you're here:

  • FleetReading can't distinguish "a wave met nobody new" from "maxWaves ran out" — with the header fix a 16-replica fleet exhausts maxWaves at exactly 16 probes and still reports as if it had converged. Returning that bit would let fleetDrift decline to report arrivals off a reading that never converged.
  • It's also the answer to the bot's P1 ask for a departure arm. Measured on a fleet of 8 with a 6 s idle gap and no deploy, today's sampling produces three spurious arrivals and three spurious departures in the same pair of readings. A departure arm on top of this refuses more runs without catching more deploys; it only becomes safe once a reading is complete.

Class: regression, introduced here. Blocking in my view — the fix is one line and I measured it — though it can't fire on a target whose fleet fits inside one wave's connections.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] This was the right call and I reproduced the measurement independently before acting on it — a local server reporting the socket each request arrived on gives 4, 5, 5 cumulative distinct sockets over three waves of four, and 4, 8, 12 with Connection: close. Fixed in 7586367: the header is on every probe, with the numbers recorded next to it so deleting it is not a silent change, and a unit test asserts every probe carries it.

The wave loop keeps its role, now with something to widen: it stops when a wave discovers nothing new and every expected replica has answered, capped at maxWaves.

Comment on lines +724 to +736
if (!args.emitWorkload) {
openingFleet = await readFleet(bootUrl);
console.log(describeFleet(openingFleet, bootUrl));
let straddle = fleetStraddle(openingFleet);
if (straddle.length) {
// Refusing here costs a probe. Refusing at the close costs the window.
console.error(
`\nRefusing to start: ${straddle.join('; ')}.\n` +
`Wait for the rollout to finish — aws ecs describe-services reports\n` +
`deployments[0].rolloutState, which must read COMPLETED — and re-run.`,
);
process.exit(1);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] An opening reading that answered nothing walks straight past this block and then guarantees a refusal at the close — the outcome this block exists to buy out of.

fleetStraddle on an empty reading finds no builds, returns [], and the run proceeds after printing "answered none of N probes". At the close, either the target still isn't answering and fleetDrift refuses on its closing-silence arm, or it is answering and the build arm fires with a blank where the starting build should be:

the host build moved:  at the start, main-AAAA.js (0.0.0+abcd) at the close

That's [...seenBefore].join(' and ') over an empty set. Nothing moved — the opening probe failed.

Refuse here when openingFleet.responses === 0, alongside the straddle check. "Refusing here costs a probe. Refusing at the close costs the window" applies to this case at least as strongly as to a straddle.

Class: regression. Blocking — as it stands the harness can print a sentence naming a build change that did not happen.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Fixed in 7586367, though not with a start-up refusal. fleetDrift now returns no findings when either reading answered nothing, and describePin reports build: not pinned for an opening reading that never took — a run cannot be refused for failing a check it never passed, and a target that does not serve the host app at all is a legitimate thing to point this at.

The malformed message is gone with it: the build arm can no longer run against an empty seenBefore. Covered by a test asserting an empty opening reading produces no findings and a not pinned summary line.

Comment on lines +256 to +262
): string[] {
if (after.responses === 0) {
return [
`nothing answered the closing probe (${after.probes} attempts), so the ` +
`deployment cannot be confirmed to have held still`,
];
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Silence at the close is routed into refusal, but it says the pin is unknown, not that the deployment moved — and describePin already has vocabulary for unknown.

What it costs is the whole run. Measured against a target that accepts the connection and never answers, the closing probe takes 20.1 s (two waves × the 10 s per-probe timeout) and then every number the run collected is discarded — nothing is written to a file, so there is no recovering it. And the runs likeliest to land here are the ones this harness exists for: /_standby is served by the same Koa app on the same event loop as _federated-search, so a run that drives the server into multi-second event-loop lag is a run whose closing probe can miss its 10 s window.

This is a decision rather than a fix I'd impose, but I'd take the other branch: print the summary with build: not pinned — the closing probe went unanswered, so these numbers are not confirmed to be one deployment. Refusal is the right answer to "the deployment moved"; this is "we could not tell", and the operator has already paid for the window either way.

Class: regression, non-blocking — but it wants an answer before merge.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Agreed, and changed in 7586367. Close-side silence now routes to describePin, which prints build: <build> at the start, NOT CONFIRMED at the close — nothing answered N closing probes, so a deploy inside this window would not have been seen, and the summary follows. pinIsConfirmable is the predicate.

The trade decided it: the likeliest target to go quiet at the close is the one this harness has just spent an hour saturating, nothing is persisted, and silence says the pin is unknown rather than that the deployment moved. Quoting the numbers as one build's without saying so would be the worse failure, which is why the line is loud rather than absent.

Comment on lines +158 to +164
for (let result of results) {
if (!result) {
continue;
}
responses++;
replicas.set(result.replicaId ?? UNIDENTIFIED_REPLICA, result.build);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] replicas.set(UNIDENTIFIED_REPLICA, …) is last-write-wins, so on a target that sets no container header a reading keeps one build rather than the builds. That's less than the comment on UNIDENTIFIED_REPLICA promises — "its readings compare builds and never turnover" — because there is only ever one build left to compare. Against a no-header target alternating two bundles:

replicas.size = 1 | builds seen = [ 'main-AAAAAAAA.js' ]
fleetStraddle  = []
Deploy pin: host build main-AAAAAAAA.js (0.0.0+AAAAAAAA), 1 replica over 8 probes.

which is the bot's P2, confirmed. Either carry the set of builds seen separately from the replica map, so distinctBuilds and fleetStraddle still work without replica identity, or narrow the comment to say such a target compares nothing.

Class: regression, non-blocking — it can only reach a multi-replica target off ECS.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Confirmed against the module and fixed in 7586367 — a reading now keeps a builds map alongside replicas, so builds survive independently of replica identity and a header-less fleet mid-rollout is caught. The constant's comment now claims only what the code does.

Comment on lines +1522 to +1534
test('a deployment that identifies no replicas never reports turnover', function (assert) {
// A local stack sets no container header. Filing every response under
// one key is what keeps that from reading as a fleet of one that keeps
// being replaced.
let same = build('main-QY-TXAfv.js', '0.0.0+b707165c');
assert.deepEqual(
fleetDrift(
reading({ unidentified: same }),
reading({ unidentified: same }),
),
[],
);
});

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] This passes with the rule it names deleted. Both readings carry the same key, so !before.replicas.has(id) already excludes it and id !== UNIDENTIFIED_REPLICA is never consulted — I removed that clause from fleetDrift and re-ran these inputs: still [], still green.

The shape the clause actually decides is an identified start and an unidentified close:

before { 'task-a': X }   after { 'task-a': X, 'unidentified': X }
  with the clause   -> []
  clause deleted    -> ["1 replica answered the closing probe…"]

Asserting [] on that pair pins the rule. Worth importing UNIDENTIFIED_REPLICA rather than spelling 'unidentified' here too, so the key and the rule that reads it can't drift apart.

Class: regression (test added here), non-blocking.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Right — the clause was redundant given identical keys, so the test passed with the rule deleted. Rewritten in 7586367 to assert the property that can actually break: a header-less deployment whose build moved reports exactly one finding, the build move, and no phantom fleet change. That fails if builds are ever filed per replica, which is the mistake the key exists to prevent. A second test keeps the held-still case.

Comment on lines +1293 to +1300
test('a preloaded chunk is not mistaken for the entry bundle', function (assert) {
// The document names dozens of chunks and one entry. Reading a chunk
// would pin the run to a file that changes on a different schedule from
// the build a browser boots.
let served = parseServedBuild(bootDocument());
assert.strictEqual(served.bundle, 'main-CThYvmXC.js');
assert.notStrictEqual(served.bundle, 'chunk-Bv0JxpqV.js');
});

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] notStrictEqual(served.bundle, 'chunk-Bv0JxpqV.js') can't fail against this fixture: the chunks are <link rel="modulepreload"> and entryBundle only scans <script> tags, so the fixture parses identically with both preload links deleted. The shape that would discriminate is a chunk carried on a module script ahead of the entry — which does return chunk-Bv0JxpqV.js today, since entryBundle takes the first type="module" tag with a src.

Either build the fixture that way, or drop this test: as written it restates the assertion in the test above it.

Class: non-blocking.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Fixed in 7586367. The fixture now carries the inline script the served document has, ahead of the preloads and the entry, and the test asserts the ordering it depends on before asserting the result — so it fails for a parser that takes the first script tag with a src as well as for one that scans every assets/*.js in the document.

Comment on lines +1113 to +1114
process.on('SIGINT', () => void finish());
setTimeout(() => void finish(), args.minutes * 60000);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Ctrl-C no longer stops a run promptly, and there is no second chance: by the time a second SIGINT arrives running is already false, so finish() returns immediately and the process stays alive until the closing probe finishes. Against a target that has stopped answering that is 20.1 s, measured — and then the drift arm throws the summary away.

Either give the closing probe a tighter timeout when it runs under SIGINT, or let a second SIGINT process.exit outright.

Class: regression, non-blocking.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Fixed in 7586367 — the SIGINT handler now calls process.exit(130) when running is already false, so the first Ctrl-C ends the run and reads the deployment one last time, and a second one leaves immediately.

Comment on lines +23 to +25
// visible even when the task definition did not change, and a replaced
// task is a cold one — its own reason to discard the window, and the way
// three runs were lost to cold containers before this check existed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] "…and the way three runs were lost to cold containers before this check existed" dates the comment to the change that introduced it — a reader a year from now has no way to place "before". The reason a replaced task invalidates a window stands without it; end the sentence at "its own reason to discard the window".

Class: non-blocking.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Fixed in 7586367 — the clause is gone. What replaced it states the standing reason rather than the occasion: a replaced task is a cold one, which invalidates the window on its own.

Each reading is a sample of the fleet, and the comparison refuses a run on
what the two samples differ by — so a sample that misses a replica invents a
deploy. Three changes make the sample sound enough to carry that decision.

Every probe opens its own connection. Concurrency alone widens the sample
only within a wave: the pool's sockets are free again by the time the next
wave is issued, and undici prefers a free socket to a new one, so a reading
converged on the few connections its first wave opened. Measured against a
server that reports the socket each request arrived on, three waves of four
reached 4, 5, 5 distinct sockets; asking each probe to close its connection
reaches 4, 8, 12. A fleet larger than one wave was refused with nothing
deployed, because the run's own traffic widened the pool and the closing
reading reached replicas the opening one structurally could not.

The closing reading is told which replicas answered at the start and keeps
probing while any of them has yet to answer, which is what lets a replica be
reported as departed rather than merely unseen. A fleet that shrank carried a
different share of the load through the rest of the window, so it is drift
alongside a fleet that grew; a replaced task is both at once and says so.

Builds are recorded apart from the replicas that served them. A target that
sets no container header files every response under one key, and the builds
were overwriting one another until only the last probe's survived — so a
fleet mid-rollout reported one build and the run proceeded across two.

Two readings that could not be taken are no longer treated as deploys. An
opening probe that answered nothing leaves the run unpinned rather than
manufacturing a build that moved from nothing; a closing probe that answered
nothing reports the pin as unconfirmed and still prints the summary, because
the likeliest server to go quiet at the close is the one the harness has
spent an hour saturating.

A second Ctrl-C now exits immediately rather than waiting out the closing
probe.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@habdelra

Copy link
Copy Markdown
Contributor Author

@codex review

[Claude Code 🤖] Both findings are addressed in 7586367, along with a connection-sampling defect that had to be fixed first for the vanished-replica arm to be sound. Worth a fresh pass on readFleet's wave loop and on fleetDrift's two set differences.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7586367a41

ℹ️ 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".

Comment on lines +215 to +217
let html = await response.text();
if (!response.ok) {
return undefined;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve replica IDs from non-2xx replies

When an opening replica still answers the closing probe but _standby transiently returns a non-2xx response, this discards its metadata ID along with the unusable document. If any other replica succeeds, after.responses remains positive and fleetDrift consequently reports the responding replica as having left, falsely refusing the run as deployment movement. Record the replica's presence independently of whether its boot document is usable, or make such a closing reading unconfirmable.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Good catch, and it is the worst-shaped false refusal left in here: the target likeliest to return a transient error at the close is the one this harness has just spent an hour saturating. Fixed in e6c878a.

A reading now keeps the replicas apart from the builds — replicas is a set of every id that answered at all, builds a map filled only from usable documents — so a 502 contributes its identity and nothing else, and fleetDrift no longer sees a departure.

Your second suggestion turned out to be needed as well, not instead: with identity preserved but pinIsConfirmable still keyed on responses > 0, a close where every replica errored would have compared an empty build set against the opening one, found nothing missing and reported the pin as held. It now returns pinIsReadable(after), so that close reads NOT CONFIRMED and the summary still prints. Both halves have a test, and each fails on its own when the other is in place.

Comment on lines +896 to +898
let closingFleet = openingFleet
? await readFleet(bootUrl, { expect: openingFleet.replicas.keys() })
: undefined;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Quiesce load generators before the closing probe

When the duration expires while a reader is awaiting primeConnections or a writer is awaiting modelCall, those loops can still dispatch a search or write after running becomes false because they do not recheck it after those awaits. Since finish immediately starts the closing reading while the detached loops continue, a fast post-stop request can be recorded in the summary after some or all closing probes have already completed, leaving that measurement outside the deployment-pin window. Stop or await the load loops before taking the closing reading.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Correct on both loops. Fixed in e6c878a by not starting measured work once the clock has stopped: the reader loop rechecks running after primeConnections in both the modelled and subscribed branches, and the writer loop rechecks it after modelCall. Priming opens a connection per query and the forwarded call is a round trip of its own, so either could resume well after the run ended and then dispatch a full batch of searches, or a write.

A request already in flight when the clock stops is left alone deliberately — it began inside the window the pin covers, and its latency is part of what the run measured. What the fix removes is work that had not started yet.

These loops live in the entry script's top-level scope, so they are not reachable from the unit suite as it stands; this one is verified by reading rather than by a test, unlike the pin changes.

…lock

A replica answering `502` has still said it is there, and the fleet comparison
needs nothing else from it. Its id was being discarded along with the unusable
document, so a transient error at the close read as a task that had left and
refused the run. A reading now records which replicas answered apart from
which builds they served — the replicas a set, the builds a map — and a
closing reading counts as confirming the pin only when it brought back a boot
document, rather than whenever something answered. Without that second half,
a close where every replica errored would compare an empty build set against
the opening one, find nothing missing, and report the pin as held.

The reader and writer loops now recheck the clock after the awaits that can
outlast it. Priming opens a connection per query and a forwarded call is a
round trip of its own; a loop that resumed from either after the run ended
went on to dispatch a whole batch of searches, or a write, into a window the
closing pin no longer covers.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@habdelra

Copy link
Copy Markdown
Contributor Author

@codex review

[Claude Code 🤖] Both of the last round's findings are addressed in e6c878a — a reading now keeps replica identity apart from the builds it read, a close that brings back no boot document reports an unconfirmed pin rather than an unchanged one, and the reader and writer loops recheck the clock after the awaits that can outlast it.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e6c878acc0

ℹ️ 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".

Comment on lines +911 to +912
let closingFleet = openingFleet
? await readFleet(bootUrl, { expect: openingFleet.replicas.keys() })

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Stop the initial render before probing the close

If SIGINT or a short duration fires while the initial readerLoop is awaiting primeConnections, this closing probe starts, but the initial-render path at lines 610-611 subsequently dispatches its full search batch without rechecking running. The new post-priming guards cover only reruns, so this remaining path can still add measurements after the deployment-pin window has begun closing; guard the initial render as well or await the reader loops before taking this reading.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Right, the earlier guards covered the re-runs and left the largest batch a reader issues unguarded. Fixed in c8e239a — the first render is wrapped in the same running check.

I kept it as a guard rather than awaiting the reader loops from finish: a reader can be inside an unbounded search when the clock stops, awaiting it would hold the closing probe for as long as that search takes, and a request that started inside the window belongs in the numbers anyway. The property being protected is that no new measured work starts after the window closes, which the guard gives directly.

Comment on lines +191 to +192
if (result.build) {
builds.set(buildLabel(result.build), result.build);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Exclude responses that do not identify a build

When a 200 response contains an error page or otherwise lacks both identifiers, parseServedBuild still returns a truthy object, so this inserts unknown bundle (unknown version) as a distinct build. If another probe returns a valid document, an opening reading falsely triggers fleetStraddle; at the close, the same mixed responses make fleetDrift report that the host build moved and discard an otherwise stable run. Only add the parsed build when at least one identifier is present.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Reproduced before fixing: a fleet where one replica answers 200 with an interstitial and the other with the real document records ['unknown bundle (unknown version)', 'main-AAAA.js (0.0.0+aaaa)'] and fleetStraddle fires, so the run is refused before it starts. Fixed in c8e239a — a build is recorded only when the document named a bundle or a version, which also makes pinIsReadable exactly "this reading holds a build".

It flushed out a test-side version of the same confusion: the suite's reading() helper could construct a reading holding a build with neither identifier, which the production path can no longer produce. The helper now filters the same way, so the fixtures stay reachable states.

A 200 carrying an error page parses to a build with neither identifier in it,
and recording that stood a second "build" beside the real one: a fleet where
one replica answered with an interstitial read as a straddle before the run,
and as a host build that moved at the close, out of a deployment that never
changed. Only a document that named a bundle or a version is a build now,
which also makes a readable pin exactly a reading that holds one.

The first render is no longer started after the clock stops either. It is the
largest batch a reader issues, priming can outlast a short run or a Ctrl-C,
and the guards added for the re-runs did not cover it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@habdelra

Copy link
Copy Markdown
Contributor Author

@codex review

[Claude Code 🤖] Both addressed in c8e239a — a build is taken only from a document that named a bundle or a version, and the reader's first render is guarded by the same clock check as its re-runs.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🎉

Reviewed commit: c8e239aeb2

ℹ️ 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".

@github-actions

github-actions Bot commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Host Test Results

    1 files      1 suites   2h 12m 26s ⏱️
4 895 tests 4 881 ✅ 14 💤 0 ❌
4 910 runs  4 896 ✅ 14 💤 0 ❌

Results for commit c8e239a.

Realm Server Test Results

    1 files    244 suites   1h 25m 41s ⏱️
3 647 tests 3 647 ✅ 0 💤 0 ❌
3 698 runs  3 698 ✅ 0 💤 0 ❌

Results for commit c8e239a.

@habdelra
habdelra requested a review from a team September 18, 2026 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants