Keep the first model-list answer after a restart - #168
Merged
Merged
Conversation
Measured on the dev stack after merging PR #167: the model list stayed unknown for ten minutes after every restart. - Both image channels read with offset reset latest. A group is only assigned its partition after a delay - a new group, or one waiting for a dead member's session to expire after a restart - and a message sent in that window was skipped. Both now reset to earliest. Replay is harmless: answers are idempotent, handled in order, and an answer about an image no longer configured is dropped. - The refresh timer's first tick ran at startup, before the Kafka emitter was connected, and logged an injection ERROR on every start. It now waits one minute; the startup ask already covers boot.
Review of PR #168: reading from the earliest offset made replay possible, so every record on these channels must survive arriving late or twice. - Image questions carry the time they were asked; answers echo it. The catalogue keeps the answer to the newest question (V82), so a replayed older answer cannot roll the list and pin back. The worker skips questions older than 15 minutes instead of pulling an image nobody runs any more ahead of the current question. - A sign-in start sent while the worker was rejoining was never read, leaving the row PENDING for ever. Both sign-in channels now read from earliest. A start carries the operator's press time: the worker opens no unit once the wait has run out, gives a late start only the time left, and ignores a replay for a unit still running. The orchestrator re-sends an unanswered start every 30 seconds while its wait lasts, then fails it as sign_in_not_started, and closes a prompt two minutes past its code's expiry. - Dead-lettered harness records now replay onto their own topics instead of cs.commands. - The agent image pins Codex 0.156.1, which lists the gpt-6 models. Flags, API-key login and its file shape were re-checked; the --json event stream waits for a paid run (UNVERIFIED section B). - The build step says why no model can be picked when every one lacks a price; a select with every option disabled looked broken.
Third review of PR #168 found four ways a late record could still act. - A start with no press time predates this change, so it can only be a replay; the worker gave it the full wait and could reopen a finished sign-in. It now opens no unit. - A late start reported EXPIRED. With two workers, a late copy on one would fail a sign-in the other was running. A start too late to open a unit now opens nothing and reports nothing; the orchestrator ends unclaimed rows from the row's own clock. - The retry pass failed rows through the ordinary path, which also accepts PROMPTED, so a prompt landing between its read and its write was ended as not started. The timeout now requires the row to still be PENDING when the write runs. - Undated image questions were answered and undated answers could replace a cached row. Both are now treated as replays: skipped, and never replacing. No test reproduces the third race deterministically; the conditional write is the fix and is stated rather than proved.
Fourth review of PR #168: when a second worker's create hit the container name another worker owned, and the lookup that followed failed or found nothing, the error escaped as UNIT_FAILED. With two workers that ended a sign-in the other one was running and the operator could still approve. The conflict itself proves the unit is held, so it now always means AlreadyClaimed; the lookup only adds detail. A holder that no longer exists is resolved by the orchestrator's deadlines.
Fifth review of PR #168: any failure before a worker owns the unit - a connection reset, a timeout on create - became UNIT_FAILED. With two workers that ended a sign-in the other one was running. A failure before ownership is now logged and dropped; the orchestrator re-sends the start. Because a broken worker now reports nothing, a sign-in that shows no code within five minutes is ended as sign_in_not_started, instead of after the fourteen minutes a person may take. A worker shows the code within a minute; the rest is room for a first pull of the agent image.
Sixth review of PR #168: the orchestrator ended a sign-in with no code after five minutes, while the worker still admitted its start for the fourteen minutes a person may take. A worker recovering at minute six opened a unit for a row already ended, and a start admitted at 4:50 printed its code after the row was gone. Start now carries startWithinSeconds (four minutes). The worker opens a unit only if it can print the code before that window closes; the orchestrator ends an unclaimed row two minutes after it, leaving room for the code to cross the bus. One number, sent, instead of two that could drift. The comment that justified the old deadline with an image pull was wrong - the sign-in unit pulls nothing - and is gone.
Seventh review of PR #168: the worker checked the start window only at admission and took the approval wait as a duration before the unit started. A unit admitted in time that started slowly, or whose process was paused, could publish a code after the row had been ended, and a slow start stretched the fourteen-minute approval limit. Start now exposes promptDeadline and approvalDeadline, both from the press. The worker waits for the code no longer than the prompt deadline, rechecks it before publishing - a late unit is destroyed and reports nothing - and measures the code's expiry and its exit wait against the approval deadline at the moment it uses them. The worker reads a replaceable clock so a test can make a unit start slowly. The orchestrator stops re-sending once the start window has closed; the row stays PENDING until its deadline, since a code may be on its way.
Eighth review of PR #168: the expiry added a duration taken from one clock reading to a later reading, so after a pause the countdown ran past the worker's real deadline. The expiry is now the sooner of the vendor's lifetime from one reading and the approval deadline itself.
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
1 participant
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.
Why
Found on the dev stack right after merging #167. After each restart the harness model list stayed
unknown until the next 10-minute refresh:
harness-image-commands-in(run worker) andharness-image-results-in(orchestrator) usedauto.offset.reset: latest. A consumer gets its partition only after a delay (a new group, or arejoin waiting for the dead member's session to expire). The question or answer sent in that window
was skipped. Observed: the worker answered with status OK, and the orchestrator's group showed no
committed offset and never recorded it.
HarnessCatalogues.refreshticked at startup before its Kafka emitter was connected:SRMSG00019: Unable to connect an emitter with the channel harness-image-commands-out.What changed
reset: earliest. Replay is harmless: answers are idempotent, consumed inorder, and answers about an image no longer configured are dropped.
@Scheduled(..., delayed = "1m")on the refresh. The startup ask still covers boot.Verification
:spire-orchestrator:testand:spire-run-worker:testpass.their levels.
Not in this PR
The same startup race hits the existing
WorkItemOutbox#publishtimer (work-events-out). That isolder than #167 and left for a separate change.