feat(g01): observe bounded idle drain ordering - #72
Conversation
|
@codex review |
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: 951e1b8b10
ℹ️ 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".
| if phase == "drain" { | ||
| return d.drain(ctx, s.setID) |
There was a problem hiding this comment.
When the intended prerequisite exists—one registered, idle runner—the preceding d.owned call classifies the scale-set statistics as workUnresolved because statisticsWork rejects any nonzero registered/idle count, so Run returns ErrQuarantine before reaching this branch. Consequently the newly exposed drain phase cannot execute for the exact state required by validDrainIdlePrerequisite; dispatch it through drain-specific ownership validation before the generic no-worker gate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Drain is routed before generic work quarantine; TestDriverRoutesDrainBeforeNoWorkerStatisticsQuarantine and real pinned-SDK Driver integration verify the idle prerequisite is reachable.
Resolution evidence at correction commit 5d715c4: https://github.com/1XP-AI/gh-runnerd/blob/5d715c486c959ae67ca615c4eaea3e7e89ede556/docs/evidence/g01-idle-drain.md . All 241 committed files match the two independently reviewed archive contents. Exact-head focused race verification passed (1.896s). Fresh exact-head Codex review and CI remain separate gates; this reply does not treat staleness as resolution or authorize merge/live work.
| case <-ctx.Done(): | ||
| return obs, ErrQuarantine |
There was a problem hiding this comment.
When the phase deadline expires or the caller cancels while a poll is in flight, this return releases the held body but does not wait for the l.Run goroutine started above. That goroutine can then resume parsing and enter journaledDrainClient, which persists an intent before noticing the canceled context, while drain concurrently closes the session and the command may close the journal; this can corrupt the ordering evidence and leave late ACK/acquisition activity outside the phase lifecycle. Cancel and join runResult before returning from every ctx.Done() path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cancellation releases the held body and joins the listener. Cancellation-before-snapshot, after-intent, no-late-effects, and race tests pass; no claim of revoking bytes already sent remotely.
Resolution evidence at correction commit 5d715c4: https://github.com/1XP-AI/gh-runnerd/blob/5d715c486c959ae67ca615c4eaea3e7e89ede556/docs/evidence/g01-idle-drain.md . All 241 committed files match the two independently reviewed archive contents. Exact-head focused race verification passed (1.896s). Fresh exact-head Codex review and CI remain separate gates; this reply does not treat staleness as resolution or authorize merge/live work.
| if response.Body == nil { | ||
| response.Body = http.NoBody | ||
| } | ||
| response.Body = &drainHeldBody{source: response.Body, release: h.release} |
There was a problem hiding this comment.
When the first poll returns a bodyless 202 Accepted/no-message response, the client can classify it from the status and close the body without ever calling Read (the synthetic session in drain_test.go follows exactly this pattern). Because drainHeldBody.Close is not gated, this wrapper does not actually hold that response from the listener, yet responseHeld is set to true and an absent first poll is allowed to produce an observed journal record. Gate both reads and close/response delivery, or do not promote status-only responses to observed evidence.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Both Read and Close remain release-gated. TestDrainHeldBodyHoldsCloseUntilRelease passes and bodyless/unknown no-message samples remain inconclusive.
Resolution evidence at correction commit 5d715c4: https://github.com/1XP-AI/gh-runnerd/blob/5d715c486c959ae67ca615c4eaea3e7e89ede556/docs/evidence/g01-idle-drain.md . All 241 committed files match the two independently reviewed archive contents. Exact-head focused race verification passed (1.896s). Fresh exact-head Codex review and CI remain separate gates; this reply does not treat staleness as resolution or authorize merge/live work.
|
Independent Luna/max protocol and security reviews of exact head 951e1b8 both return FAIL. Focused offline tests passed but did not cover all required behavior. Blocking review items: persist observed request/work and rejected idle-prerequisite uncertainty before crash boundaries so later stale-zero cleanup cannot clear them; reject missing/replaced after-runner identity and no-message/unknown-counter samples as conclusive evidence; join listener lifecycle and fence effects against phase cancellation (including upstream WithoutCancel behavior); validate duplicate/wrong ACK/acquisition identity before remote effects; demonstrate Driver.Run/OpenDrainSession composition with the actual pinned SDK, not only a synthetic session. These source/control-flow findings have been assigned to the sole writer for executable reproduction before fixes; they are not being represented as already-executed failing tests. Codex P1 threads r3965776815, r3965776826 and r3965776832 remain merge blockers until reproduced resolution and fresh review. The original undefined-symbol compile failure is not acceptable behavioral TDD evidence; chronology must be corrected honestly rather than relabeled. Reviewers excluded work after the shared checkout advanced to f44f5f9. No merge or live qualification is authorized by passing CI. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f44f5f9b36
ℹ️ 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".
| func (c controllerApproval) needsVerification() bool { | ||
| return slices.ContainsFunc(c.Phases, func(phase string) bool { | ||
| return phase == "before-ack" || phase == "after-ack" || phase == "before-acquire" || phase == "acquire-loss" | ||
| return phase == "before-ack" || phase == "after-ack" || phase == "before-acquire" || phase == "acquire-loss" || phase == "drain" |
There was a problem hiding this comment.
When a least-privilege paired-terminal approval contains create, drain, inspect, and cleanup, this change makes the broker treat it as verification-capable and accept it, but the downstream ValidatePairedApprovals and pairedPreparationReady checks still recognize only the four older verification phases. The broker can therefore approve and issue the paired handoff only for the controller to reject the same approval; add drain to those paired-side verification checks as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Paired approval validation recognizes drain. Exact-head TestValidatePairedApprovalsAcceptsDrainVerification passes (0.471s).
Resolution evidence at correction commit 5d715c4: https://github.com/1XP-AI/gh-runnerd/blob/5d715c486c959ae67ca615c4eaea3e7e89ede556/docs/evidence/g01-idle-drain.md . All 241 committed files match the two independently reviewed archive contents. Exact-head focused race verification passed (1.896s). Fresh exact-head Codex review and CI remain separate gates; this reply does not treat staleness as resolution or authorize merge/live work.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 945371e1ec
ℹ️ 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".
| if o == nil || o.Version != drainObservationVersion || (o.Outcome != drainOutcomeObserved && o.Outcome != drainOutcomeInconclusive) || o.InitialCapacity != drainInitialCapacity || o.WithdrawnCapacity != drainWithdrawnCapacity || o.ServerReceipt != drainServerReceiptUnproven || o.Sequence <= 0 || o.ObservedAt.IsZero() || len(o.Ordering) > 8 { | ||
| return false | ||
| } | ||
| if o.Boundary != drainBoundaryRequestWritten && o.Boundary != drainBoundaryResponseBeforeWrite && o.Boundary != drainBoundaryUnresolved || !validDrainPoll(o.Poll, drainInitialCapacity) || !validDrainPoll(o.NextPoll, drainWithdrawnCapacity) || !validDrainSnapshot(o.Before, o.Before.Set) || !validDrainSnapshot(o.After, o.Before.Set) { |
There was a problem hiding this comment.
When the canary runner disappears or re-registers with a new ID between the before and after reads, this validation still accepts the observation because each snapshot independently permits either a nil runner or any positive runner associated with the scale set. drain can therefore persist Outcome == "observed", and replay retains it without uncertainty, even though the owned idle-runner identity was lost during the experiment; compare Before.Runner and After.Runner for exact equality (and require both for an observed outcome), otherwise mark the result inconclusive.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Observed evidence requires the exact same nonnil owned runner before/after and the one-owned-idle prerequisite. Runner replacement/missing identity and real FileJournal invalid-proof regressions pass.
Resolution evidence at correction commit 5d715c4: https://github.com/1XP-AI/gh-runnerd/blob/5d715c486c959ae67ca615c4eaea3e7e89ede556/docs/evidence/g01-idle-drain.md . All 241 committed files match the two independently reviewed archive contents. Exact-head focused race verification passed (1.896s). Fresh exact-head Codex review and CI remain separate gates; this reply does not treat staleness as resolution or authorize merge/live work.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e954cfe1a
ℹ️ 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".
| if e.Operation == "drain" { | ||
| s.workObserved = true |
There was a problem hiding this comment.
If the process exits after the durable session-close result—or after the completed after-snapshot reads—but before the final drain observation is recorded, replay never enters this branch: the drain phase itself does not set workObserved or uncertain, and every effect intent may already be balanced. A later approved cleanup can therefore pass authorizePhase and call DeleteScaleSet even though the drain ran without a durable outcome; mark the phase uncertain before remote polling or make replay quarantine a drain phase lacking its observation.
AGENTS.md reference: AGENTS.md:L8-L8
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The durable drain phase remains pending until exactly one matching valid observation; missing/repeated/interrupted phase, wrong identity/sequence, and unrelated uncertainty cannot discharge it. Crash and FileJournal reopen tests pass.
Resolution evidence at correction commit 5d715c4: https://github.com/1XP-AI/gh-runnerd/blob/5d715c486c959ae67ca615c4eaea3e7e89ede556/docs/evidence/g01-idle-drain.md . All 241 committed files match the two independently reviewed archive contents. Exact-head focused race verification passed (1.896s). Fresh exact-head Codex review and CI remain separate gates; this reply does not treat staleness as resolution or authorize merge/live work.
| stats, err := newDrainStatistics(current.Statistics) | ||
| if err != nil || stats != before.Statistics { |
There was a problem hiding this comment.
When the session response contains a non-nil RunnerScaleSet whose identity, update setting, or statistics disagree with the idle before-snapshot, this check accepts the session as long as the top-level Statistics still match. The existing probe path explicitly incorporates RunnerScaleSet.Statistics as work evidence, so ignoring it here can let the drain poll, ACK, and acquire while the session itself reports existing or busy work, invalidating the observation; validate the embedded set and require its statistics to agree before starting the listener.
AGENTS.md reference: AGENTS.md:L12-L12
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Session open checks embedded scale-set identity, labels, update setting and statistics against the idle snapshot. TestDrainRejectsEmbeddedSessionStatisticsMismatch and pinned-SDK integration pass.
Resolution evidence at correction commit 5d715c4: https://github.com/1XP-AI/gh-runnerd/blob/5d715c486c959ae67ca615c4eaea3e7e89ede556/docs/evidence/g01-idle-drain.md . All 241 committed files match the two independently reviewed archive contents. Exact-head focused race verification passed (1.896s). Fresh exact-head Codex review and CI remain separate gates; this reply does not treat staleness as resolution or authorize merge/live work.
| obs, runErr := runDrainListener(ctx, journaled, setID, hook) | ||
| closeErr := d.effect(ctx, "session-close", nil, func(call context.Context) (Event, error) { |
There was a problem hiding this comment.
When the first ACK or acquisition returns an error—or acquisition returns a mismatched response—the journal wrapper records an unknown/reservation and runDrainListener returns ErrQuarantine, but this path still unconditionally calls session.Close. Closing the session after an ambiguous acquisition can alter or requeue the outstanding work and destroys the known session that the existing acquire-loss contract deliberately retains; only close after outcomes that are known safe, and retain the session on quarantine.
AGENTS.md reference: AGENTS.md:L12-L12
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Ambiguous listener effects retain the session and durable uncertainty rather than unconditionally closing. The independent archive reviews verified retained-session/zero-close behavior.
Resolution evidence at correction commit 5d715c4: https://github.com/1XP-AI/gh-runnerd/blob/5d715c486c959ae67ca615c4eaea3e7e89ede556/docs/evidence/g01-idle-drain.md . All 241 committed files match the two independently reviewed archive contents. Exact-head focused race verification passed (1.896s). Fresh exact-head Codex review and CI remain separate gates; this reply does not treat staleness as resolution or authorize merge/live work.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
|
Exact f482d24 independent reviews: security F1-F8 pass the corrected archived offline reproductions, including durable pre-ACK reservation, stale-zero cleanup refusal, runner/session identity, callback ordering and actual pinned-SDK drain integration. Protocol review additionally reproduced remaining gaps: unknown next-poll statistics and an empty statistics object can still be classified as observed; cancellation during durable ACK-intent append can precede an inner call; early snapshot cancellation omits its fixed marker. These deeper tests mean overall independent approval remains FAIL despite the narrower passing controls. Required fixes are assigned to the sole writer with focused failing-before-fix evidence; no claim is made that local cancellation can revoke a remotely accepted operation. F9 evidence chronology also remains unresolved: retrospective reproductions must not be described as preceding the correction without trustworthy earlier evidence. Root CI run34332963834 failed make fmt-check on sdk.go; formatting correction requires fresh exact-head CI. No merge or live qualification. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02ba76466d
ℹ️ 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".
| if obs.Boundary == drainBoundaryRequestWritten && obs.ResponseHeld && obs.Poll.Message == drainMessagePresent && obs.Poll.StatsKnown && obs.Poll.ACK == drainResponseSucceeded && obs.Poll.Acquisition == drainResponseSucceeded && obs.NextPoll.Message == drainMessageAbsent { | ||
| obs.Outcome = drainOutcomeObserved |
There was a problem hiding this comment.
When the first message response has internally consistent but contradictory runner counters—for example, zero registered/idle runners—newDrainStatistics accepts it and this predicate can still promote the run to observed after ACK/acquisition. The before and session snapshots prove exactly one registered idle runner, and docs/evidence/g01-idle-drain.md explicitly classifies contradictory counters as inconclusive, so require the poll's runner partition to remain consistent with that prerequisite before recording successful G01 evidence.
AGENTS.md reference: AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
First, withdrawn and after runner partitions must agree with the owned one-idle proof; legitimate job counter changes remain allowed. Contradictory first/next-poll and real FileJournal non-owned-proof tests pass.
Resolution evidence at correction commit 5d715c4: https://github.com/1XP-AI/gh-runnerd/blob/5d715c486c959ae67ca615c4eaea3e7e89ede556/docs/evidence/g01-idle-drain.md . All 241 committed files match the two independently reviewed archive contents. Exact-head focused race verification passed (1.896s). Fresh exact-head Codex review and CI remain separate gates; this reply does not treat staleness as resolution or authorize merge/live work.
| return h.inner.RoundTrip(req) | ||
| } | ||
|
|
||
| trace := &httptrace.ClientTrace{WroteRequest: func(info httptrace.WroteRequestInfo) { |
There was a problem hiding this comment.
When the first capacity-1 GET uses the keep-alive connection established while opening the session and encounters a network error, http.Transport may transparently retry this idempotent bodyless request. Each physical attempt invokes WroteRequest, but wroteOnce hides every callback after the first and no duplicate successful write marks the hook invalid; the retried request also retains its already-built capacity-1 header despite SetMaxRunners(0). Two old-capacity polls can therefore reach the service while the final response is still promoted to observed, so count and quarantine duplicate write callbacks or prevent transport-level retries.
AGENTS.md reference: AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Each physical write is accounted for on BOTH polls. Duplicate/error/transparent retry traces cannot promote observed evidence; first/second real HTTP retries and 4x4 matrix pass.
Resolution evidence at correction commit 5d715c4: https://github.com/1XP-AI/gh-runnerd/blob/5d715c486c959ae67ca615c4eaea3e7e89ede556/docs/evidence/g01-idle-drain.md . All 241 committed files match the two independently reviewed archive contents. Exact-head focused race verification passed (1.896s). Fresh exact-head Codex review and CI remain separate gates; this reply does not treat staleness as resolution or authorize merge/live work.
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e1b2144cb
ℹ️ 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".
| if !second { | ||
| return nil, ErrQuarantine | ||
| } | ||
| response, err := h.inner.RoundTrip(req) |
There was a problem hiding this comment.
When the second, capacity-zero GET uses a reused connection and encounters a retryable transport failure, http.Transport may transparently replay this idempotent bodyless request. The current correction counts WroteRequest callbacks only in the first-poll branch; this branch delegates directly to the inner transport, so multiple zero-capacity polls can reach GitHub while the journal still records one poll-zero and promotes the result to observed. Instrument and quarantine duplicate physical writes for the second poll as well.
AGENTS.md reference: AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The withdrawn poll now uses the traced request path too; exactly one successful physical write per poll is required. TestDrainListenerRejectsWithdrawnPollPhysicalRetry and the both-poll matrix pass.
Resolution evidence at correction commit 5d715c4: https://github.com/1XP-AI/gh-runnerd/blob/5d715c486c959ae67ca615c4eaea3e7e89ede556/docs/evidence/g01-idle-drain.md . All 241 committed files match the two independently reviewed archive contents. Exact-head focused race verification passed (1.896s). Fresh exact-head Codex review and CI remain separate gates; this reply does not treat staleness as resolution or authorize merge/live work.
| if e.Operation == "drain" { | ||
| // The phase record is durable intent to observe a live session. | ||
| // A crash before its final observation must not reopen cleanup. | ||
| s.uncertain = true |
There was a problem hiding this comment.
Every real drain journal includes this phase record, so even a fully successful final drain observation replays with uncertain == true: the phase sets the flag here, while the observed-outcome branch only avoids adding more uncertainty and never clears the phase-local fence. This contradicts the documented distinction that only inconclusive drain results retain uncertainty and causes a successful experiment to remain classified like an interrupted one; track and discharge this specific fence when a valid observed record closes the phase without clearing unrelated uncertainty.
AGENTS.md reference: AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Replay tracks a phase-local fence with positive explicit SetID and journal phase sequence. Only its exact valid one-idle observation discharges it; unrelated uncertainty, pending intents, work/reservations remain. Real FileJournal close/reopen and negative histories pass.
Resolution evidence at correction commit 5d715c4: https://github.com/1XP-AI/gh-runnerd/blob/5d715c486c959ae67ca615c4eaea3e7e89ede556/docs/evidence/g01-idle-drain.md . All 241 committed files match the two independently reviewed archive contents. Exact-head focused race verification passed (1.896s). Fresh exact-head Codex review and CI remain separate gates; this reply does not treat staleness as resolution or authorize merge/live work.
| | Security F5 / Protocol F5 counters and integration | known counters require nonnegative internally consistent busy/idle partition; unknown values cannot masquerade as zero. `TestDrainRequiresKnownConsistentStatisticsAndControlledMessage` and `TestDriverDrainThroughPinnedSDKAndPollHook` pass (`82d0d7d`). | | ||
| | Protocol F3 cancellation/WithoutCancel | phase context is bound through the wrapper, checked before every ACK/acquisition/poll effect, and cancellation/deadline/quarantine writes a fixed marker even when close/after inspect fails. `TestDrainRejectsEffectsAfterCancellationAndRecordsMarker` passes (`82d0d7d`). | | ||
| | Protocol F4 duplicate/wrong callbacks | exact phase, identity, ACK-before-acquire and one-shot state are validated before inner effects; wrong/duplicate callbacks make zero additional inner calls. `TestDrainRejectsDuplicateOrWrongEffectsBeforeInnerCall` passes (`82d0d7d`). | | ||
| | Protocol F6 cancellation race and early snapshot | durable intent is followed by a cancellation fence immediately before the bounded SDK call; a canceled before-snapshot path records its fixed marker. The fence does not claim to revoke bytes already accepted by a remote service. `TestDrainCancellationAfterIntentRejectsEffect` and `TestDrainCancellationBeforeSnapshotRecordsMarker` pass on the correction head. | | ||
| | Protocol F9 TDD chronology | the compile-only red and retrospective base/repro executions are now labeled candidly; no later archive reproduction is represented as pre-implementation evidence. | |
There was a problem hiding this comment.
The correction matrix ends without recording the two latest Codex findings concerning contradictory poll counters and transparent poll retries as Codex resolutions with their finding URLs, reproduction results, and resolution evidence. Although related tests now exist, the repository explicitly requires those per-finding records after fixes, so this head does not yet satisfy the documented exact-head review gate and must not be treated as merge-ready until the matrix is completed.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The durable correction matrix now includes full finding URLs, actual red/green commands/results and rollback scope, including both earlier retry/counter findings and subsequent phase/idle-proof corrections. Original missing TDD history remains candidly disclosed with a separately recorded maintainer exception.
Resolution evidence at correction commit 5d715c4: https://github.com/1XP-AI/gh-runnerd/blob/5d715c486c959ae67ca615c4eaea3e7e89ede556/docs/evidence/g01-idle-drain.md . All 241 committed files match the two independently reviewed archive contents. Exact-head focused race verification passed (1.896s). Fresh exact-head Codex review and CI remain separate gates; this reply does not treat staleness as resolution or authorize merge/live work.
|
Coordinator checkpoint for 6e1b214: all four hosted CI checks passed (run 34340970782). Two independent Luna/max archive reviews passed the three targeted corrections and their focused prior-boundary tests, including actual first-poll transparent HTTP retry fixtures. Those passes do NOT cover newly reported second-poll retry and successful-phase fence findings. Latest exact-head Codex review reports actionable findings #72 (comment), #72 (comment), and missing durable evidence #72 (comment). A sole Luna/max writer is assigned behavioral reproduction before minimal corrections and evidence updates. Merge remains held. PR body now honestly distinguishes initial compile-only failure from meaningful TDD; unavailable original behavioral-red chronology remains a historical process gap, not retroactively repaired by regression tests. No live resources or credentials were changed. |
|
Exact-head checkpoint: 9904048 has all four CI checks successful (run 34408989415). Independent Luna/max protocol review passed the prior six corrections; security review remains in progress. The fresh Codex review at 2026-09-09T22:01:40Z raises four NEW P1 findings: non-EOF poll-body read errors (r3973406578), physical ACK binding (r3973406571), strict runner snapshot decoding (r3973406564), and strict drain-session decoding (r3973406553). Merge remains blocked. A sole Luna/max writer is assigned meaningful red-first reproductions and minimal adapter corrections, plus a bounded audit of the full drain evidence boundary to avoid piecemeal lossy-decoding gaps. No live operation is authorized or performed; parent G01 live evidence remains unresolved. Completed protocol-review terminal has been released. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88d37abea8
ℹ️ 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".
| h.wroteRequest = true | ||
| } | ||
| h.mu.Unlock() | ||
| if notify && info.Err == nil && h.onRequestWritten != nil { | ||
| h.onRequestWritten() |
There was a problem hiding this comment.
When the first response becomes available while the WroteRequest callback is still executing, wroteRequest is already true even though onRequestWritten has not yet completed SetMaxRunners(0). The response-first branch can therefore treat the boundary as proven and release the held body, allowing parsing, ACK, and acquisition before capacity withdrawal completes and falsely promoting the run to observed. Record a separate withdrawal-completed fact after the callback returns and require it before releasing or proving this boundary.
AGENTS.md reference: AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correction in 64c6fce, retained in current clean head d85213a with the additional exact HTTPS Actions queue-host fix b3d45cb.
The hook now waits for withdrawal callback completion before normal response release and boundary proof; deterministic blocked-callback and cancellation tests cover the ordering.
Timestamped implementer red/green evidence and rollback are in docs/evidence/g01-idle-drain.md. Coordinator independently ran:
cd experiments/g01-scaleset
GOTOOLCHAIN=go1.26.8 go test -race ./livecanary -run '^(TestPinnedSDKDrainRequiresApprovedHTTPSQueueHost|TestValidDrainQueueURLRequiresExactApprovedHostPort|TestPinnedSDKDrainAcceptsUnrelatedRunnerMetadata|TestDrainListenerDoesNotReleaseBeforeWithdrawalCompletes|TestDrainListenerCancellationWhileWithdrawalBlockedDoesNotDeadlock|TestDriverDrainThroughPinnedSDKAndPollHook)$' -count=1 -timeout=180sPASS, package1.533s, no race report. All evidence is offline. Current-head independent protocol/security review, fresh Codex and CI remain pending; keeping this thread open until final evidence audit.
| ScaleSetID *int `json:"runnerScaleSetId"` | ||
| } `json:"value"` | ||
| } | ||
| if DecodeStrict(data, &w) != nil || w.Count == nil || w.Value == nil || *w.Count < 0 || *w.Count > 1 || *w.Count != len(*w.Value) { |
There was a problem hiding this comment.
When the distributed-task agent response includes normal non-identity metadata such as status or version fields, DecodeStrict rejects the entire payload because its decoder disallows fields absent from this three-field value struct. The pinned SDK normally ignores such extra fields, but the new wire reader now quarantines both before and after snapshots before the drain listener can run. Retain the recursive duplicate-key check while decoding only the bounded identity fields with an unknown-field-tolerant unmarshal, as the scale-set capture already does.
AGENTS.md reference: AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correction in 64c6fce, retained in current clean head d85213a with the additional exact HTTPS Actions queue-host fix b3d45cb.
Runner metadata decoding now tolerates unrelated fields while retaining recursive duplicate/case-fold rejection and required count/value identity constraints.
Timestamped implementer red/green evidence and rollback are in docs/evidence/g01-idle-drain.md. Coordinator independently ran:
cd experiments/g01-scaleset
GOTOOLCHAIN=go1.26.8 go test -race ./livecanary -run '^(TestPinnedSDKDrainRequiresApprovedHTTPSQueueHost|TestValidDrainQueueURLRequiresExactApprovedHostPort|TestPinnedSDKDrainAcceptsUnrelatedRunnerMetadata|TestDrainListenerDoesNotReleaseBeforeWithdrawalCompletes|TestDrainListenerCancellationWhileWithdrawalBlockedDoesNotDeadlock|TestDriverDrainThroughPinnedSDKAndPollHook)$' -count=1 -timeout=180sPASS, package1.533s, no race report. All evidence is offline. Current-head independent protocol/security review, fresh Codex and CI remain pending; keeping this thread open until final evidence audit.
|
Exact-head checkpoint 88d37ab: all four CI checks pass, and coordinator race tests for the prior four corrections pass (1.575s). Fresh Codex review at 2026-09-09T22:56:00Z raises two P1s: normal extra runner metadata rejection (r3973755454) and withdrawal completion racing the response boundary (r3973755448). A sole Luna/max writer is assigned red-first reproductions and minimal corrections; independent reviewers remain read-only on the immutable archive. Merge remains blocked, with no live operation performed. Parent G01 live evidence remains unresolved. |
|
Checkpoint cb2c1ba: source64c6fce0f9f86ece48a7b28ee51b1aef9774f2cb fixes runner metadata compatibility and withdrawal-completion race, with timestamped red/green evidence. An independent security P1 remains: session queue URL can use the general API origin instead of an exact approved Actions host. The same implementation session is assigned that focused red-first follow-up before final reviews, avoiding redundant intermediate review launches. Both completed independent-review terminals were released. Unknown session-open remains locally fenced; a durable known session ID/reservation must not be claimed when strict decoding failed. No live operations or merge performed; parent G01 still requires authorized live evidence. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d85213a212
ℹ️ 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".
| return runErr | ||
| } | ||
| closeErr := d.effect(ctx, "session-close", nil, func(call context.Context) (Event, error) { | ||
| if err := session.Close(call); err != nil { |
There was a problem hiding this comment.
If the pinned SDK sends the close DELETE to the wrong set/session path and that endpoint returns success, this direct session.Close call records session-close as completed, replay clears the outstanding session ID, and the drain can return an observed result even though its actual session remains open. Use the existing terminal-session-close wire capture to require the exact session DELETE and 204 response before persisting success.
AGENTS.md reference: AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correction pushed in 05b5fff; current review head 116beda. Timestamped baseline red and green evidence is recorded in docs/evidence/g01-idle-drain.md. Coordinator independently ran GOTOOLCHAIN=go1.26.8 go test -race ./livecanary -run '^(TestPinnedSDKDrainBindsAcquireToPhysicalRequestBody|TestPinnedSDKDrainBindsSessionCloseToPhysicalDelete|TestDriverDrainThroughPinnedSDKAndPollHook)$' -count=1 -timeout=180s: PASS (1.829s), including TestPinnedSDKDrainBindsSessionCloseToPhysicalDelete. Two independent Luna max reviews and a fresh exact-head Codex review are pending; this reply does not assert a clean verdict or authorize merge/live operations.
| wire = &baselineWireCapture{stage: "acquire", setID: setID, queue: queue, allowedHosts: baselineWireAllowedHosts(c.d.Approval, apiHost)} | ||
| call = wire.context(call) | ||
| } | ||
| got, err = c.inner.AcquireJobs(call, slices.Clone(ids)) |
There was a problem hiding this comment.
If the pinned SDK serializes a different runner-request ID than ids in the acquisition POST while the endpoint returns the expected count/value, the capture validates only method, path, host, query, and response, so both accepted.matches(ids) and slices.Equal(got, ids) pass while the remote call may have acquired different work. Pass the expected IDs into this wire capture and strictly compare the bounded POST body before forwarding it.
AGENTS.md reference: AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correction pushed in 05b5fff; current review head 116beda. Timestamped baseline red and green evidence is recorded in docs/evidence/g01-idle-drain.md. Coordinator independently ran GOTOOLCHAIN=go1.26.8 go test -race ./livecanary -run '^(TestPinnedSDKDrainBindsAcquireToPhysicalRequestBody|TestPinnedSDKDrainBindsSessionCloseToPhysicalDelete|TestDriverDrainThroughPinnedSDKAndPollHook)$' -count=1 -timeout=180s: PASS (1.829s), including TestPinnedSDKDrainBindsAcquireToPhysicalRequestBody. Two independent Luna max reviews and a fresh exact-head Codex review are pending; this reply does not assert a clean verdict or authorize merge/live operations.
|
Checkpoint d85213a: all four CI checks pass, both independent Luna/max passes validate the metadata, withdrawal-completion and exact Actions queue-host corrections, and both reviewer terminals are released. Fresh exact-head Codex review at 2026-09-09T23:41:25Z identifies two remaining P1 request-side gaps: physical acquisition POST body binding (r3974007599) and exact session-close DELETE binding before clearing the session fence (r3974007586). A sole Luna/max writer is assigned meaningful red-first real-SDK reproductions and minimal existing-adapter fixes. Merge remains blocked, no live operation performed, full parent G01 live evidence remains unresolved. |
|
@codex review |
|
Checkpoint for exact head 116beda: all four Public CI checks succeeded (run 34419651240). Coordinator focused race regressions passed (1.829s), and correction evidence is linked in replies to r3974007599/r3974007586. Two fresh independent Luna max reviews are active; a fresh Codex review was requested for this head and its verdict remains pending. A possible asynchronous request-body Read/Close lifetime issue is assigned to the security reviewer for reproduction; it is not yet a confirmed finding. No merge or live operations authorized by these offline results; G01 remains open. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 116beda04d
ℹ️ 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".
| if c == nil || c.stage != "acquire" || len(c.requestIDs) == 0 || !c.target(req) { | ||
| return nil |
There was a problem hiding this comment.
When an SDK regression or request wrapper changes the acquisition host, path, set ID, or query, the !c.target(req) case returns success here, so baselineRequestCaptureTransport still sends the request and only quarantines after the remote side effect. A wrong-set acquisition can therefore reserve unrelated work even though the adapter claims to be the final pre-network boundary. Fresh evidence beyond the earlier request-body finding is that invalid bodies are now stopped before forwarding, while invalid targets explicitly bypass that guard; return an error for a marked acquisition whose target does not match.
AGENTS.md reference: AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
| wire := &baselineWireCapture{stage: "terminal-session-close", setID: setID, sessionID: sessionID} | ||
| call = wire.context(call) |
There was a problem hiding this comment.
When an approval contains multiple allowed Actions hosts and the SDK or a transport wrapper sends this close to the wrong allowed origin, the terminal-session-close capture checks only the path suffix and query, not the scheme/host. A 204 from that other origin therefore satisfies wire.observed(), clears the durable session ID, and permits an observed result while the real session remains open. Fresh evidence beyond the earlier set/session-path finding is that the corrected capture still receives no expected origin here; bind it to the exact session-open endpoint before accepting close success.
AGENTS.md reference: AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
|
Exact-head Codex review completed for 116beda at 2026-09-10T00:20:25Z with two new P1 findings: r3974234277 (marked acquisition target bypasses pre-network validation), r3974234284 (session close is not bound to the expected endpoint origin). All four CI checks passed, but this head is NOT merge-ready. A sole Luna max implementation worker is assigned to reproduce both before minimal fixes; the two independent baseline reviewers continue archive-only checks, including asynchronous request-body lifetime. The historical clean verdict for 21fe1f is stale and does not authorize this head. No live operation or merge has occurred. |
|
Safe handoff: source 1d25d31 and evidence f08e9f3 are pushed. The writer reports red/green fixes for acquisition target preflight, session-close origin binding, and standard request-body lifetime/concurrency; exact commands and chronology are in docs/evidence/g01-idle-drain.md. Coordinator has accepted the handoff and released the implementation/review terminals, not approved merge. Maintainer prioritizes workflow-efficiency issue73: no new broad review cycle or duplicate local full-suite run is being started now. This head still needs independent delta acceptance, fresh exact-head Codex verdict, and completed CI; G01 live evidence and parent goal remain open. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f08e9f3e0b
ℹ️ 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".
| if c.stage == "session-open" { | ||
| if !c.target(req) { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
When the pinned SDK or a transport wrapper changes the session-creation host, path, set ID, method, or query, this branch returns success and baselineRequestCaptureTransport still forwards the POST. A successful wrong-target request can therefore create an orphaned session for another scale set; OpenDrainSession rejects it only after the remote side effect and discards the returned session identity, leaving no way to close it. Reject a marked session-open request that does not match c.target(req) before calling the inner transport.
AGENTS.md reference: AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0e8a2ec70
ℹ️ 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".
| } else if c.stage == "acquire" { | ||
| suffix += "acquirejobs" | ||
| if len(c.allowedHosts) == 0 || !slices.Contains(c.allowedHosts, r.URL.Host) { | ||
| return false |
There was a problem hiding this comment.
When an approval lists multiple Actions hosts, or when api.github.com is added by baselineWireAllowedHosts, an acquisition redirected to any one of those hosts still passes this membership check. If that origin has the same numeric scale-set ID and returns the expected count/value, the request can reserve unrelated work and be recorded as successful; the fresh evidence beyond the earlier target fix is that the current capture stores the exact session-open origin but acquisition validates only the broader allowlist. Require the acquisition origin to equal the captured session origin.
AGENTS.md reference: AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
| if !c.target(req) { | ||
| if baselineSessionOpenRequestCandidate(req) { | ||
| return c.rejectRequest(req) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
When the marked session-creation request is mutated to a path that no longer contains /runnerscalesets/—for example /sessions on an allowed origin—this predicate classifies it like bootstrap traffic and forwards the state-changing POST before the response-side capture can quarantine it. The fresh evidence beyond the prior resolution is that the new candidate check covers only one route family, so the claimed pre-network target fence is still bypassable; explicitly allow the known bootstrap requests and reject every other marked non-target request.
AGENTS.md reference: AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6357865735
ℹ️ 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".
| if !c.target(req) { | ||
| if baselineAcquireRequestCandidate(req) { | ||
| return c.rejectRequest(req) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
When a wrapper rewrites the marked acquisition POST to a path that contains neither the case-sensitive /_apis/runtime/runnerscalesets/ fragment nor the /acquirejobs suffix, this branch still forwards it even though c.target(req) has already rejected it. For example, a case-folded wrong-set acquisition route can reach a case-insensitive service and reserve unrelated work before the response is quarantined. Fresh evidence beyond the prior acquisition-target finding is that the new candidate predicate still leaves this route-family escape; unlike session opening, acquisition has no bootstrap traffic to preserve, so every marked non-target request should fail before the inner transport.
AGENTS.md reference: AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
| if reader, ok := d.API.(drainScaleSetWireReader); ok { | ||
| wire := &baselineWireCapture{stage: "set-observe", setID: setID} | ||
| set, err = reader.drainGetScaleSet(call, setID, wire) |
There was a problem hiding this comment.
When an approval contains multiple allowed Actions hosts, this evidence-bearing capture records no expected origin, while the set-observe target check accepts any HTTPS request whose path merely ends in the scale-set suffix. Because newSDKTransport permits every approved Actions host, a mutated before or after request can read the same numeric set ID from a different origin (or tenant path), and matching identity/counters from that response can still support an observed drain. Fresh evidence beyond the earlier strict snapshot-response finding is that the response is now decoded strictly but its physical request origin is never captured or corroborated; bind both snapshots to the same expected runtime origin before using them as proof.
AGENTS.md reference: AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
| c.origin = origin | ||
| c.mu.Unlock() | ||
| return nil |
There was a problem hiding this comment.
When the pinned SDK or an intervening wrapper serializes a different owner in the marked session-creation POST, the exact target and origin checks pass and this branch forwards the request without inspecting its body. The service can therefore create a live session for the wrong owner; the later strict response comparison quarantines it but OpenDrainSession discards the returned session object, leaving the controller without an identity it can safely close. Validate the bounded session-open request body before the inner transport, as the acquisition path already does for its request IDs.
AGENTS.md reference: AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
| Date: 2026-09-10. This correction addresses the two independent private review | ||
| findings on exact head c0e8a2e: acquisition was bound to the approved-host set | ||
| rather than the session-open origin, and the marked session-open classifier | ||
| could forward route-family escapes or reject an organization named |
There was a problem hiding this comment.
This final correction describes the acquisition-origin and session-open route findings only as independent private-review findings and does not record either Codex discussion URL, even though both findings were fixed in this head. Fresh evidence beyond the earlier correction-matrix finding is that these corrections were appended after that matrix was completed, so its earlier resolution does not cover them. The repository requires a finding URL, reproduction result, and resolution evidence for every addressed Codex finding; add the two latest URLs so this head can satisfy the documented exact-head review gate.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
1 participant
Summary
Implements issue #71 as an experiment-only drain observation phase on pinned
github.com/actions/scaleset v0.4.0.SetMaxRunners(0).WroteRequestboundary; response-before-write/timing misses remain inconclusive.TDD / verification
The initial undefined-symbol compile failure was not meaningful behavioral red. Original pre-implementation behavioral-red evidence is unavailable; later retrospective reproductions do not repair that history. The maintainer explicitly approved this historical process exception only (decision: #71 (comment)). Current corrections retain actual red-before-fix evidence and unchanged review/CI/live gates.
Correction head
5d715c486c959ae67ca615c4eaea3e7e89ede556has 241 files byte-identical to the snapshot independently reviewed by two Luna/max agents. Coordinator re-ran focused drain/replay/FileJournal/security/pinned-SDK race tests on that exact commit: PASS (1.896s). Exact commands and finding-by-finding evidence remain indocs/evidence/g01-idle-drain.md. Fresh exact-head Codex and hosted CI are pending; this is not merge readiness or live qualification.Passed:
cd experiments/g01-scaleset && go test ./...cd experiments/g02-auth && go test ./...bash scripts/check-offline-experiments.shmake fmt-check build vet test test-raceThe synthetic listener tests are offline protocol evidence only; G01 live qualification remains unresolved. See
docs/evidence/g01-idle-drain.md.Closes #71