amadan
A send deadline is one absolute instant fixed before the lease or authorization, on both paths; the first review's findings are recorded
Both reviewers rejected this branch at 1772e840. Two findings they shared are fixed here: - The quantity request's deadline was the send window added to the clock as read after the lease was claimed. It is now fixed from a wall reading taken before the claim, capped by the replay cutoff, and never rebuilt. - No invoice request carried a send deadline. Every one now does, fixed before its authorization is read. Each has a reproduction with a suspended sender and was mutation-checked. The rest of the review is recorded as open in DISPOSITIONS-late-request-fencing.md, including the limit both reviewers found in the design's premise: an idle-connection limit does not bound a partially written request. Nothing on the launch branch calls this code. Billing is off.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
8 files changed,
+452
−2
docs/superpowers/specs/CODEX-FINDINGS-late-request-fencing-review1.md+85 −0docs/superpowers/specs/DISPOSITIONS-late-request-fencing.md+35 −0docs/superpowers/specs/OPUS-REVIEW-late-request-fencing-review1.md+149 −0internal/console/billinginvoice.go+39 −0internal/console/billinginvoice_test.go+49 −0internal/console/billingquantity.go+31 −2internal/console/billingquantity_test.go+60 −0internal/console/billingservice.go+4 −0
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-late-request-fencing-review1.md b/docs/superpowers/specs/CODEX-FINDINGS-late-request-fencing-review1.md| new file mode 100644 |
| index 000000000..e506b492b |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-late-request-fencing-review1.md |
| @@ -0,0 +1,85 @@ |
| +1. **P1 — The quantity deadline moves forward when a suspended sender resumes.** |
| + `internal/console/billingquantity.go:1062–1064` at `1772e840`. |
| + |
| + `window` is a duration derived from the original claim, but `WithSendBy` receives **the current time plus that duration**, rather than the claim’s absolute expiry. Suspend the VM after `claimQuantitySend` succeeds and before line 1063. Another console can fence and retire the operation; after the fence expires, the resumed sender receives a fresh deadline and opens a fresh connection. |
| + |
| + The outer two-minute context does not reliably rescue this on a suspension where the monotonic clock stops: with more than 15 seconds remaining monotonically, the HTTP client’s shorter timeout can supply a new child deadline calculated after resumption. |
| + |
| + **Smallest acceptable fix:** carry `until.Add(-billingClockTolerance)` unchanged into `WithSendBy`, capped by the operation’s replay cutoff. Add coverage for suspension between the successful claim and deadline construction. |
| + |
| +2. **P1 — Invoice-line requests never receive the operation’s absolute send deadline.** |
| + `internal/console/billinginvoice.go:758–761,943` and `internal/billingstripe/client.go:137,199` at `1772e840`. |
| + |
| + There is no `WithSendBy` in the invoice engine. Suspend after `authorizeBillingPost` returns but before `http.Client.Do` establishes its timeout. Another console sends and acknowledges the line. Resume after its key has expired, while that invoice is still a draft: the old invocation can acquire a fresh client deadline and create a duplicate line on a fresh connection. |
| + |
| + The five-minute pass context helps ordinary elapsed-time cancellation, but has the same suspended-monotonic-clock/shorter-child-deadline problem described above. Requests without `WithSendBy` are accepted; the transport checks their effective context deadline, which does not necessarily retain the original authorization’s wall-clock expiry. |
| + |
| + **Smallest acceptable fix:** supply an immutable deadline for every invoice mutation, bounded by the authorized lease, pass expiry and `StartedAt + billingIdempotencyReplayLimit`. Preserve it through request construction and retries. |
| + |
| +3. **P1 — An idle-connection limit does not establish the claimed complete-request delivery bound.** |
| + `internal/billingstripe/transport.go:138–164` and `internal/billingstripe/transport_test.go:23–25` at `1772e840`. |
| + |
| + The check establishes only that the **sampled** wall time precedes the supplied deadlines. It neither makes serialization atomic nor sets a deadline on delivery of the last request byte: |
| + |
| + | Freeze point | What the code establishes | |
| + |---|---| |
| + | Before the wall-clock sample | An already-expired, correctly preserved deadline causes refusal before HTTP bytes are written. | |
| + | After sampling, including before comparison | The comparison uses the old sample. Protection now depends on the established connection being rejected remotely. | |
| + | After the check, before writing | A sufficiently long continuously idle interval is covered only by the provider premise. Local cancellation cannot execute while the process is stopped. | |
| + | After the request line/headers, before the complete body | A timeout for an unused connection does **not** establish a timeout for a partially received request. | |
| + | After bytes enter the kernel, before the last byte leaves | SIGSTOP and GC pauses do not stop kernel transmission or retransmission. Those bytes can continue arriving and refresh an inactivity timer. VM suspension can defer queued transmission until resume. | |
| + | After the provider receives the request | Closing the connection does not retract the request or establish that processing stopped. | |
| + |
| + `Request.Write` and TLS/TCP delivery can involve multiple writes. Several pauses separated by progress also invalidate “check time + one idle interval,” even if every individual idle interval is bounded. The fake server configures `ReadHeaderTimeout`; it establishes no body-read bound. |
| + |
| + **Smallest acceptable fix:** replace the claimed bound with an explicit, supported bound covering complete request ingress, including partial bodies and queued transmission. Add partial-write and repeated-pause cases, and keep both paths disabled while that provider premise remains unestablished. An empty-connection probe alone cannot cover this. |
| + |
| +4. **P1 — The invoice argument assumes the key was first used after this sender’s check.** |
| + `docs/superpowers/plans/2026-10-01-late-request-fencing.md:41–44` at `1772e840`. |
| + |
| + The asserted `t2 ≥ t1` excludes retries. A line can first execute at hour 0, lose its response, and be retried at hour 21. A suspension of three hours after the retry’s check can deliver another execution after retention expires—even with a provider idle limit of four hours, satisfying the plan’s `T_idle < 24h` premise. |
| + |
| + **Smallest acceptable fix:** calculate the allowance from the key’s earliest possible execution, rather than from another copy assumed to arrive later. With a 22-hour replay cutoff, the delivery bound must fit inside the remaining approximately two hours, less clock and scheduling margins. Make the sandbox acceptance threshold enforce that bound; an arbitrary configurable wait is insufficient. |
| + |
| +5. **P2 — Recovery after 22 hours discards the void/paid outcome before recording the review.** |
| + `internal/console/billinginvoice.go:723–728,1096,1135–1158` at `1772e840`. |
| + |
| + Suppose the process dies after finalizing the wrong invoice and returns after the replay cutoff. `recoverFinalize` calls `finalizedOrVoided`, which can successfully void it but returns the invoice ID, `"void"` and the original verification error. `perform` returns that error before `acknowledge`. |
| + |
| + The same happens when the customer has already paid, or when a definitive void refusal is followed by a successful read. The operation remains unacknowledged and the intended durable review is never filed. Subsequent visits repeat the mismatch. The added recovery tests resume within minutes and miss this branch. |
| + |
| + **Smallest acceptable fix:** handle a recovered object accompanied by a verification error through the same acknowledge-and-review logic as the immediate response. Add cases beyond 22 hours for successful void, lost void response, customer payment and definitive refusal. |
| + |
| +6. **P2 — Incomplete or mismatched invoice data can authorize voiding a correct invoice.** |
| + `internal/console/billinginvoice.go:1124–1141` and `internal/billingstripe/invoices.go:65–78` at `1772e840`. |
| + |
| + After any verification failure, the destructive-action guard checks only invoice ID, customer and subtotal inequality. The decoder accepts an omitted or null `subtotal` as zero. Thus an otherwise identifiable response for a correct, positive-value open invoice, missing that field, triggers a void. Failures of mode, currency or calculation metadata also do not prevent the void. |
| + |
| + **Smallest acceptable fix:** require complete monetary fields and independently validate the invoice’s full billing identity before treating subtotal inequality as authority to void. Malformed or identity-mismatched responses should produce a durable review without mutation. |
| + |
| +7. **P2 — The sandbox idle test can report success without measuring an idle connection.** |
| + `internal/billingstripe/sandbox_fence_test.go:461–489` at `1772e840`. |
| + |
| + A DNS, TCP or TLS failure before `afterConnect` runs returns an error; if the subsequent GET finds no marker, the test logs `VERIFIED`. The GET’s status and object identity are not checked, so an error response with no metadata also satisfies that assertion. There is no unheld positive control establishing that this mutation would otherwise succeed. |
| + |
| + The direct `RoundTrip` uses `context.Background()` and has no response deadline, so another outcome is an indefinitely blocked probe. |
| + |
| + **Smallest acceptable fix:** require evidence that connection establishment and the intended hold occurred; validate a successful post-probe read; add an unheld mutation control; distinguish remote closure from unrelated failures; and bound observation time without treating the local timeout as provider evidence. |
| + |
| +8. **P3 — Several transport tests do not exercise the behavior their names or comments claim.** |
| + `internal/billingstripe/transport_test.go:40–57,130–138,173–195` at `1772e840`. |
| + |
| + The “after its last check” hold actually runs **before** the check. The no-proxy test checks the transport type and `KeepAlive` field but never exercises proxy settings. The response-timeout test starts a 20 ms deadline before dialing and never waits for handler entry: under scheduling delay, it can pass on a dial timeout without exercising response-error normalization. |
| + |
| + **Smallest acceptable fix:** introduce synchronization at the actual boundaries under test, exercise proxy environment settings against an observable proxy, and start response cancellation only after the server confirms receipt. Replace sleep-based ordering with explicit barriers where possible. |
| + |
| +**Claims the code contradicts or exceeds** |
| + |
| +- The plan’s line 60 says both workers pass their lease ends; the invoice engine does not, and the quantity worker reconstructs a relative deadline. Lines 37–44 and `transport.go:54–57` overstate the delivery bound and retention argument. |
| +- The plan’s line 62 says the void ensures the invoice is not left payable. Failed or late voids and process death leave exposure; customer payment can win the race. The later remediation caveats correctly acknowledge this, but do not resolve the contradictory earlier claim. The void does not close item 9. |
| +- The plan’s lines 78–85 overstate test coverage. The same-visit test does not explicitly assert the intervening open state; the “lost response” fixtures return HTTP 502 rather than exercising an actual timeout; recovery beyond retention is absent. |
| +- `client.go:68` describes `HTTPStatus == 0` as meaning the request never reached Stripe. A lost response after execution also produces zero. By inspection, the relevant callers conservatively treat timeout/network errors as uncertain, rather than definitely unsent. `CodeSendWindowClosed` means this invocation wrote no HTTP request; it says nothing about earlier attempts. Callers also treat that code conservatively as pending. |
| +- I found no production bypass of the one-shot transport, redirect following, transport retry, connection handoff, TLS session-cache resumption, early-data send, or detached request-writing goroutine. The cancellation callback only closes the connection. These observations do not establish the delivery bound. |
| +- Neither the fake-provider tests nor the unrun sandbox probe establish Stripe’s behavior. No tests or network operations were run for this review. |
| + |
| +**Verdict: reject.** |
| \ No newline at end of file |
diff --git a/docs/superpowers/specs/DISPOSITIONS-late-request-fencing.md b/docs/superpowers/specs/DISPOSITIONS-late-request-fencing.md| new file mode 100644 |
| index 000000000..e80f986db |
| --- /dev/null |
| +++ b/docs/superpowers/specs/DISPOSITIONS-late-request-fencing.md |
| @@ -0,0 +1,35 @@ |
| +# Late-request fencing: review dispositions |
| + |
| +Branch `feat/carloku-late-request-fencing-20261001`. Nothing on the launch branch calls this code. Billing is off. |
| + |
| +## Review one (reviewed `1772e840`; Astra rejected, Opus rejected) |
| + |
| +Reports: `CODEX-FINDINGS-late-request-fencing-review1.md`, `OPUS-REVIEW-late-request-fencing-review1.md`. |
| + |
| +Both found the transport itself sound in what it does: no redirect, retry, connection reuse, proxy, TLS resumption or early data lets a request travel around it. Both rejected the branch. It does not close `OUTSTANDING.md` items 7 and 9 on the launch branch, and after this review it is clear that the plan as written cannot: see "The premise" below. |
| + |
| +| Finding | Disposition | Where | Reproduction | |
| +| --- | --- | --- | --- | |
| +| Both, P1: the quantity request's deadline was the send window added to the clock as read after the lease was claimed, so a sender suspended in between woke to a fresh deadline | Fixed. The deadline is one absolute instant, fixed from a wall reading taken before the claim and the lease's own end, capped by the operation's replay cutoff. | `console/billingquantity.go` `quantitySendBy` | `TestQuantitySendDeadlineDoesNotMoveWhileTheSenderIsSuspended`, `TestQuantitySendByIsTheEarlierOfTheLeaseAndTheReplayCutoff` | |
| +| Both, P1: no invoice request carried a send deadline at all | Fixed. Every request the invoice engine sends goes through one function, which fixes an absolute deadline before it reads the authorization: the earlier of the pass's end and the operation's replay cutoff. | `console/billinginvoice.go` `send`, `invoiceSendBy` | `TestInvoiceSendDeadlineDoesNotMoveWhileTheSenderIsSuspended`, `TestInvoiceSendByIsTheEarlierOfThePassEndAndTheReplayCutoff` | |
| +| Astra P1, Opus P2: the premise. See below. | Accepted, not fixed. It is a limit of the design, not of the code. | plan document | none | |
| +| Astra P1: the plan's argument for the invoice path assumes the key was first used after this sender's check; a retry at hour 21 leaves about two hours, not the provider's whole idle limit | Accepted, not fixed. The bound must be counted from the key's first possible use. Open. | plan document | none | |
| +| Both, P2: a void made on the recovery path past 22 hours is returned as an error before it is acknowledged, so it is neither recorded nor reviewed, and the visit repeats | Accepted, not fixed. Open. | `console/billinginvoice.go` `recoverFinalize`, `finalizedOrVoided` | none | |
| +| Astra P2: an invoice response with a missing subtotal, or failing on mode, currency or metadata, can authorize voiding a correct invoice | Accepted, not fixed. Open. The void must require complete monetary fields and the full billing identity. | `console/billinginvoice.go`; `billingstripe/invoices.go` | none | |
| +| Opus P2: the void uses a fixed key, so a remembered 5xx replays for 24 hours | Accepted, not fixed. Open. | `console/billinginvoice.go` | none | |
| +| Both, P2: the sandbox idle probe can log VERIFIED without having held a connection, and has no positive control | Accepted, not fixed. Open. It has never been run. | `billingstripe/sandbox_fence_test.go` | none | |
| +| Astra P3, Opus P3: transport tests that do not exercise what their names say (the hold placed before the check; the proxy never exercised; a response-timeout test that can pass on a dial timeout) | Accepted, not fixed. Open. | `billingstripe/transport_test.go` | none | |
| +| Both, claims: the plan says both workers pass their lease ends; that the void ensures the invoice is not left payable; `client.go` says status 0 means the request never reached Stripe | The first is now true. The other two are wrong and not yet corrected in the text. Open. | plan document, `client.go` | none | |
| + |
| +Mutation checks in a scratch copy, each failing its test: the quantity deadline rebuilt from the clock after the claim; the invoice request sent without its deadline. |
| + |
| +## The premise |
| + |
| +The plan rests on one sentence: a request held past the provider's idle limit is never processed, because the provider has closed the connection. Both reviewers showed that this covers less than the plan says. |
| + |
| +- It covers a sender frozen with a connection established and nothing yet written. |
| +- It does not cover a sender frozen after the request line or headers are written and before the body is complete. A limit on idle connections is not a limit on partially received requests, and nothing on this branch measures or cites one. |
| +- It does not cover bytes already handed to the kernel. A stopped process does not stop the kernel sending or retransmitting them, and a suspended machine sends them when it resumes. |
| +- Several pauses with progress between them are not bounded by one idle interval. |
| + |
| +So the wall-clock check plus the provider's idle limit bounds when a request can BEGIN to be written. It does not bound when a complete request is received. Until there is a supported provider-side bound on that (or a design that does not need one), items 7 and 9 stay open and both paths stay disabled, whatever the sandbox probe later measures. Nobody is asked to accept that risk. |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-late-request-fencing-review1.md b/docs/superpowers/specs/OPUS-REVIEW-late-request-fencing-review1.md| new file mode 100644 |
| index 000000000..b4edd7b1e |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-late-request-fencing-review1.md |
| @@ -0,0 +1,149 @@ |
| +# Review: feat/carloku-late-request-fencing-20261001 @ 1772e840 (base d5705a63) |
| + |
| +The verdict is **reject**. Two P1s are open. The quantity send deadline is computed relative to a time that moves after the lease is taken, so the item 7 window is still there. And no invoice request carries a send deadline at all, so the prevention half of item 9 is not built. The transport itself holds up: I found no redirect, retry, connection reuse, proxy, TLS resumption or early-data path around it. |
| + |
| +## Findings |
| + |
| +### P1. The quantity deadline is relative to `time.Now()`, not anchored to the lease |
| +**`internal/console/billingquantity.go:1062-1063`** |
| + |
| +`window := until.Add(-tol).Sub(claimed)`, then the deadline is `WithSendBy(ctx, time.Now().Add(window))`, plus `WithTimeout(..., window)`. `claimed` is read before `claimQuantitySend`. `time.Now()` is read afterwards. Any freeze between those two reads pushes both the send-by and the context deadline later by the length of the freeze. In production `quantityClock` is `time.Now`. |
| + |
| +- **Scenario:** the process is stopped for 30 hours after `claimQuantitySend` commits (or while it waits on the store lock). Meanwhile the lease expires, the operation is fenced and retired, and Stripe forgets the key. On waking, `SendBy` is wake time plus about the full lease minus 5 minutes. The transport check passes and the old quantity runs as new. That is item 7 exactly. |
| +- **What this contradicts:** the comment above the line says "a sender frozen here cannot send late". |
| +- **Fix:** measure the remaining window from the current clock, not from `claimed`. That also stays correct under the simulation's shifted `quantityClock`: |
| + ```go |
| + window := until.Add(-billingClockTolerance).Sub(quantityClock()) |
| + if window <= 0 { release; return done } |
| + ``` |
| + Use the same `window` for both `WithSendBy(time.Now().Add(window))` and `WithTimeout`. Add a test that runs a hook between the claim and the send and moves the clock past `until`. |
| + |
| +### P1. No invoice request carries `WithSendBy`, so item 9's prevention is not built |
| +**`internal/console/billinginvoice.go:758-761`** (`send`: `authorizeBillingPost`, then `p.post`), and the line post at about line 943. |
| + |
| +The plan's argument for invoice lines needs t2 ≥ t1: the other sender's copy runs no earlier than the frozen sender's last check. That only holds if the last check is bounded by the frozen sender's lease end. All the transport checks for these requests is the http.Client deadline, which is `Do()` start plus 15 seconds. That catches a freeze that starts inside `Do`. It catches nothing for a freeze between `authorizeBillingPost` and `Do`. |
| + |
| +- **Scenario:** engine A authorizes the `line-x` post, then the VM is suspended before `Do`. B takes over and sends `line-x` under the same key; it is acknowledged. B validates the lines and finalizes or waits. A resumes 25 hours later. `Do` computes a fresh 15-second deadline, the check passes, Stripe has forgotten the key, and a duplicate line lands on the invoice. |
| +- **What this contradicts:** |
| + - Plan line 60: "the quantity worker and the invoice engine pass their lease ends as the send deadline". The plan's own "Built so far" section correctly lists only the quantity worker. |
| + - The `finalizedOrVoided` comment, which says prevention "rests on the transport". |
| + - `transport.go:131-133`: "so every request is checked". That is true only against a deadline that starts at `Do`. |
| +- **Fix:** in `send()`, wrap `ctx` with `billingstripe.WithSendBy(ctx, <the lease or pass end authorizeBillingPost enforces> - billingClockTolerance)` before calling `p.post`. Add a test that freezes between authorize and post. |
| + |
| +### P2. A freeze in the middle of writing the request is bounded by a different, unmeasured provider limit |
| +**`internal/billingstripe/transport.go:34-38, 54-57, 164`**; **`internal/billingstripe/sandbox_fence_test.go:462-475`** |
| + |
| +`out.Write(conn)` does not send the request as one unit: |
| +- `bufio` flushes at 4 KB. |
| +- crypto/tls splits writes into records of about 1.2 KB, one write syscall each, until 128 KB has been sent on the connection. |
| +- A suspended VM can hold the second TCP segment of even a single write in the kernel's send buffer. |
| + |
| +So the provider can be left holding the headers (path, Authorization, Idempotency-Key) with part or none of the body. An invoice-item request (a 64-hex digest, metadata, tax code, period) plus headers can plausibly exceed 1.2 KB. |
| + |
| +What then limits the request is the edge's timeout for a partially received request, or a proxy that buffers the whole body before forwarding. That is not the idle limit. The sandbox probe sends zero bytes before the hold, so it measures only the state between the check and the write. |
| + |
| +The plan's statement that the idle limit is "the design's one premise" is therefore incomplete: the partial-request timeout is a second unmeasured premise. |
| + |
| +- **Fix:** |
| + 1. Name this second premise in the plan and in the `transport.go` comment. |
| + 2. Add a sandbox subtest that writes the request line and headers, holds, then writes the body. |
| + 3. Optionally, write the whole request in one TLS record. Set `DynamicRecordSizingDisabled: true` and keep requests under 16 KB with a size guard. That narrows the gap. It does not close it, because TCP segmentation remains. |
| + |
| +### P2. The void is an unjournaled, unauthorized write, and past 22 hours it is neither recorded nor reviewed |
| +**`internal/console/billinginvoice.go:1141`**, reached from the recover path at line 723. |
| + |
| +`recover` is documented as "the read-only lookup used once the idempotency window has passed". `recoverFinalize` now calls `finalizedOrVoided`, which can send `VoidInvoice`: |
| +- with no journal entry, |
| +- with no `authorizeBillingPost` (no lease check, no pass-timeout check), |
| +- with no send deadline. |
| + |
| +`perform` then does `if err != nil { return err }` without acknowledging. `finalizedOrVoided` always returns `verr` alongside the object, so the error is always non-nil here. |
| + |
| +- **Scenario 1:** the finalize answer is lost and the next visit is more than 22 hours later. That can happen because the console was down or billing was paused, or because the void kept getting a cached 5xx (next finding). The engine voids the invoice, but neither `billing_invoice_voided_wrong_total` nor a review is ever filed. Every later visit sees status `void`, takes the default case, and logs a quiet `billing_invoice_mismatch` with backoff indefinitely. |
| +- **Scenario 2:** the customer paid inside the window. `paid_wrong_total` is never recorded, so a refund owed to the customer reaches no person. |
| + |
| +This is the "void hides the evidence" case. The plan's table claims these outcomes are recorded, but that only holds on the `send` path. |
| + |
| +- **Fix:** |
| + 1. Make the void a journaled `providerOp` that goes through `send`, so it gets authorization and a send deadline. |
| + 2. In `perform`'s recover branch, when `objectID != ""` and `err != nil`, acknowledge first and then file a review, as `send` already does. |
| + 3. Add a test that advances time past `billingIdempotencyReplayLimit` for the paid case and for the voided case. |
| + |
| +### P2. The fixed void key replays a cached 5xx for 24 hours |
| +**`internal/console/billinginvoice.go:1141`** (`operationKey("void", 1)`, never renewed). |
| + |
| +Stripe stores a 500 for a request that began executing and replays it for the same key. Each later visit sends the same key and gets the same 500. That is not a definitive refusal, so the step returns pending, and the wrong invoice stays payable for the key's lifetime. The fake only stores 200 and 4xx responses; `dropRequest` and `loseResponse` are not stored. So the "retries the void on the next visit" row in the plan's table (plan line 85) is only proven against a fake that cannot produce this case. |
| + |
| +- **Fix:** |
| + 1. On a replayed 5xx (`Replayed && HTTPStatus >= 500`), read the invoice. If it is still open, renew the key (attempt+1) and journal the renewal. Voiding an open invoice is safe to repeat. |
| + 2. Make the fake store 5xx responses under the key. |
| + 3. State the 24-hour bound in the plan's table. |
| + |
| +### P2. The sandbox idle probe can log VERIFIED without holding anything |
| +**`internal/billingstripe/sandbox_fence_test.go:475-489`** |
| + |
| +Any non-nil `err` combined with unchanged metadata produces `VERIFIED`. That includes: |
| +- a dial or TLS failure before `afterConnect` runs, so the hold never happened; |
| +- `errSendWindowClosed`, so nothing was written. |
| + |
| +This is the one piece of evidence for the design's premise, so a false pass here matters. |
| + |
| +- **Fix:** set a flag in the hook and require that it ran. Require that the error is not `errSendWindowClosed` and not a dial or handshake error. Log what failed: the write, or the read and with what error. |
| +- **Wording:** drop "VERIFIED". The probe establishes "limit ≤ the configured hold, for one connection, from one network, at one time". The plan's line 70, "measures the idle limit", says more than that. |
| + |
| +### P3. Smaller issues |
| +- **`billinginvoice_test.go:1804`:** the "provider refuses the void" subtest would pass with the void deleted. Before the branch, the same flow already left the invoice open with a mismatch review and no payment. Assert `arrivals["void"] >= 1`. |
| +- **Refused void path (via `ackFinalized`):** the month is acknowledged as `open`, which is the pre-pay state, under the generic `billing_invoice_mismatch` review. The reason should say "wrong total, void refused", so a person clearing the review does not let the pay step proceed. |
| +- **`transport_test.go:130`:** `TestTransportTakesNoProxyAndSendsNoKeepalive` never sets a proxy variable, and no test exercises the TLS branch (ServerName, ALPN, handshake cancellation). Set `HTTPS_PROXY` to a listener and assert that it receives nothing. |
| +- **Stale wall clock after resume:** a VM can resume with its wall clock behind until NTP or the hypervisor corrects it. A freeze before the check is then judged against a stale clock, and the 5-minute tolerance covers drift, not a stale clock. Note it as a limit. The idle premise still covers a freeze after the check. |
| +- **`client.go` `CodeSendWindowClosed` comment:** it says only "WithSendBy". The code is also returned when the context's own deadline has passed on the wall clock. |
| + |
| +## The questions you asked |
| + |
| +### 1. What the check establishes at each freeze point |
| +The check is a wall-clock comparison, run after TCP and TLS are up, against `min(SendBy, ctx deadline)`. It establishes only that "the wall clock read below the deadline at that instant". |
| + |
| +| Where the process freezes | What bounds the request | |
| +| --- | --- | |
| +| Before the check | The check, if the right deadline is supplied: broken for quantity (first P1), absent for invoices (second P1), and dependent on a correct wall clock | |
| +| After the check, before the first byte is written | The connection idle limit (the premise the sandbox targets) | |
| +| After some bytes have left (headers, or part of the body) | The edge's partial-request timeout or proxy buffering. Not the idle limit, not measured, not in the plan | |
| +| After the last byte is in the socket | Under SIGSTOP the kernel still transmits, so the request arrives on time. Under VM suspend, unsent bytes leave on resume: idle limit if nothing had arrived, partial-request timeout if some had | |
| +| GC pause | Milliseconds; immaterial | |
| + |
| +No other way out exists: |
| +- the client has one transport and no other HTTP client calls Stripe; |
| +- `CheckRedirect` returns `ErrUseLastResponse`; |
| +- `http.Client` does not retry; |
| +- there is no connection pool and no session cache, and Go has no client 0-RTT; |
| +- the only goroutine is the `AfterFunc` close, which `stop()` removes. |
| + |
| +### 2. Who sets `WithSendBy` |
| +Only the quantity sender sets it, and wrongly (first P1). The draft, line, finalize, pay and void requests carry none. The only bound on them is `Do()` start plus 15 seconds, which says nothing about a freeze before `Do`. |
| + |
| +### 3. The void |
| +- **Customer pays first:** handled on the `send` path. Lost past 22 hours (fourth finding). |
| +- **Void fails, times out, or is late:** a cached 5xx pins it for 24 hours. A late void is unauthorized and has no deadline. |
| +- **Process dies between finalize and void:** handled within 22 hours. Past that, the void happens unrecorded. |
| +- **Void of a correct invoice:** effectively impossible. It needs a subtotal different from the frozen total after `validateDraftLines` matched them. |
| +- **Evidence:** the void invoice stays at Stripe, but the engine's own record goes silent past 22 hours. |
| +- **Claims that the void prevents a wrong charge:** none outright. Two edge on it. The test comment at `billinginvoice_test.go:1576` says "The exposure this guards against", which should say "reduces". Plan line 62, "the invoice is not left payable", conflicts with the plan's own "What the void does not do" section. |
| + |
| +### 4. Error classification |
| +No path reports a possibly-sent request as `CodeSendWindowClosed`; that error is only returned before the write. The reverse can happen: when http.Client's own timer wins the race, its timeout error replaces the error and the code becomes `CodeTimeout`. That is conservative and acceptable. A write failure after the context ended is reported as `CodeTimeout`, and other network errors as `CodeNetwork`. Both callers treat those as "may have been sent", which is correct. |
| + |
| +### 5. Tests |
| +The refused-void subtest passes without the void (P3 above). The sandbox probe can pass without the hold (P2). The transport tests rely on sleeps of 600 ms against 150 ms and 250 ms against 100 ms, but always in the safe direction, so the risk of flakiness is low. Everything in the console tests is proven only against a fake, which never stores a 5xx and never models a partial request. |
| + |
| +## Claims the code contradicts or overstates |
| +1. `billingquantity.go:1058-1061`, "a sender frozen here cannot send late". The first P1 contradicts it. |
| +2. Plan line 60, "the invoice engine pass[es] their lease ends". It doesn't. |
| +3. The `finalizedOrVoided` comment, that prevention "rests on the transport". For invoices it rests on nothing yet. |
| +4. `transport.go:54-57`, "a request reaches the provider no later than its check plus the provider's idle limit, or never". This is false for a freeze mid-write. `transport.go:34-38` states as fact what the provider does past the idle limit, which is an unmeasured premise. |
| +5. Plan line 27 says the same in the plan, and calls the idle limit the one premise. There is a second (the partial-request timeout). |
| +6. Plan line 70 and the sandbox log line, "measures" and "VERIFIED". The probe establishes an upper bound at one value and can pass vacuously. |
| +7. Plan table rows 4 and 5 and the paid row, "retries the void on the next visit" and "records billing_invoice_paid_wrong_total". Both fail past the 22-hour replay limit, and the retry is pinned for 24 hours by a cached 5xx. |
| +8. Plan line 62, "not left payable". The same plan's later section says otherwise. |
| +9. `transport_test.go:128-139`. The test name claims a proxy check that the body never makes. |
| + |
| +**Verdict: reject.** Fix the two P1s and the void's recover-path handling, and add the partial-request premise to the plan and the sandbox test, before another review. |
diff --git a/internal/console/billinginvoice.go b/internal/console/billinginvoice.go| index 4095910d2..27addc7f6 100644 |
| --- a/internal/console/billinginvoice.go |
| +++ b/internal/console/billinginvoice.go |
| @@ -90,6 +90,9 @@ type billingInvoiceHooks struct { |
| beforeLease func() error |
| beforePost func(kind string) error // between journal and request |
| beforeAck func(kind string) error // between response and acknowledgement |
| + // afterAuthorize runs between the authorization and the request, where |
| + // a suspended sender would sleep. |
| + afterAuthorize func(kind string) error |
| } |
| func (st *billingInvoiceState) ownerID() string { |
| @@ -118,6 +121,10 @@ func (st *billingInvoiceState) hook(stage string, kind string) error { |
| if st.hooks.beforeAck != nil { |
| return st.hooks.beforeAck(kind) |
| } |
| + case "authorized": |
| + if st.hooks.afterAuthorize != nil { |
| + return st.hooks.afterAuthorize(kind) |
| + } |
| } |
| return nil |
| } |
| @@ -755,9 +762,22 @@ func (r *billingInvoiceRun) send(ctx context.Context, p providerOp, op billingOp |
| if elapsed >= billingInvoicePassTimeout { |
| return context.DeadlineExceeded |
| } |
| + // The instant after which this request must not be written, fixed here, |
| + // before the authorization is read, as one absolute wall-clock instant: |
| + // the earlier of the end of this pass and the operation's replay cutoff |
| + // (past it the provider may have forgotten the key and the same request |
| + // would run as new). A sender suspended after its authorization wakes |
| + // to this deadline, not to a fresh one. The transport makes it the last |
| + // check before it writes (billingstripe, transport.go). |
| + sendBy := invoiceSendBy(billingInvoiceWall().Round(0), r.now.Add(elapsed), elapsed, time.Unix(op.StartedAt, 0)) |
| if err := r.s.store.authorizeBillingPost(r.rec, op, r.owner, r.now.Add(elapsed)); err != nil { |
| return err |
| } |
| + if err := r.b.invoice.hook("authorized", p.kind); err != nil { |
| + return err |
| + } |
| + ctx, cancel := context.WithDeadline(billingstripe.WithSendBy(ctx, sendBy), sendBy) |
| + defer cancel() |
| objectID, outcome, err := p.post(ctx, op.Key) |
| if err != nil && objectID == "" { |
| var stripeErr *billingstripe.Error |
| @@ -1325,3 +1345,22 @@ func (r *billingInvoiceRun) stepPaying(ctx context.Context) error { |
| } |
| return nil |
| } |
| + |
| +// billingInvoiceWall is the wall clock an invoice request's send deadline is |
| +// fixed against: the machine's own clock in production. A test replaces it |
| +// to stand for a sender suspended between its authorization and its send. |
| +var billingInvoiceWall = time.Now |
| + |
| +// invoiceSendBy is the absolute wall-clock instant after which an invoice |
| +// request must not be written. wall and passNow are one pair of readings |
| +// taken together, on the wall clock and on the pass's clock; elapsed is how |
| +// far into the pass they were taken. The deadline is the earlier of the |
| +// pass's end and the operation's replay cutoff, carried to the wall clock |
| +// through that pair. |
| +func invoiceSendBy(wall, passNow time.Time, elapsed time.Duration, operationStarted time.Time) time.Time { |
| + left := billingInvoicePassTimeout - elapsed |
| + if cutoff := operationStarted.Add(billingIdempotencyReplayLimit).Sub(passNow); cutoff < left { |
| + left = cutoff |
| + } |
| + return wall.Add(left) |
| +} |
diff --git a/internal/console/billinginvoice_test.go b/internal/console/billinginvoice_test.go| index 5d14bbd55..e6cb826bb 100644 |
| --- a/internal/console/billinginvoice_test.go |
| +++ b/internal/console/billinginvoice_test.go |
| @@ -1821,3 +1821,52 @@ func TestInvoiceWrongTotalWhenTheVoidFails(t *testing.T) { |
| } |
| }) |
| } |
| + |
| +// The invoice engine authorizes a request and is then suspended. The |
| +// request's deadline was fixed before the authorization was read, as one |
| +// absolute instant. When the engine wakes the deadline has passed and |
| +// nothing is written to the provider; the next visit sends. |
| +func TestInvoiceSendDeadlineDoesNotMoveWhileTheSenderIsSuspended(t *testing.T) { |
| + ix := newInvoiceFixture(t) |
| + prevWall := billingInvoiceWall |
| + t.Cleanup(func() { billingInvoiceWall = prevWall }) |
| + // As if all but 30ms of the pass had gone by on the wall when the |
| + // deadline was fixed; the sender then sleeps past what is left. |
| + billingInvoiceWall = func() time.Time { return time.Now().Add(-(billingInvoicePassTimeout - 30*time.Millisecond)) } |
| + slept := 0 |
| + ix.srv.billing.invoice.hooks = &billingInvoiceHooks{afterAuthorize: func(string) error { |
| + slept++ |
| + time.Sleep(90 * time.Millisecond) |
| + return nil |
| + }} |
| + err := ix.run(t) |
| + if slept == 0 { |
| + t.Fatalf("fixture: no request was authorized (%v)", err) |
| + } |
| + if err == nil { |
| + t.Fatal("a visit whose sender woke after its deadline reported success") |
| + } |
| + if arrivals, mutations, _ := ix.fake.counts(); len(arrivals) != 0 || len(mutations) != 0 { |
| + t.Fatalf("a sender that woke after its deadline reached the provider: arrivals %v, mutations %v", arrivals, mutations) |
| + } |
| + billingInvoiceWall = prevWall |
| + ix.srv.billing.invoice.hooks = nil |
| + if err := ix.run(t); err != nil { |
| + t.Fatalf("the next visit: %v", err) |
| + } |
| + if _, mutations, _ := ix.fake.counts(); len(mutations) == 0 { |
| + t.Fatal("the next visit sent nothing") |
| + } |
| +} |
| + |
| +func TestInvoiceSendByIsTheEarlierOfThePassEndAndTheReplayCutoff(t *testing.T) { |
| + wall := time.Date(2026, 10, 5, 12, 0, 0, 0, time.UTC) |
| + passNow := time.Date(2026, 10, 5, 12, 0, 3, 0, time.UTC) |
| + if got, want := invoiceSendBy(wall, passNow, time.Minute, passNow.Add(-time.Hour)), wall.Add(billingInvoicePassTimeout-time.Minute); !got.Equal(want) { |
| + t.Fatalf("a young operation: %s, want what is left of the pass %s", got, want) |
| + } |
| + old := passNow.Add(-billingIdempotencyReplayLimit).Add(30 * time.Second) |
| + if got, want := invoiceSendBy(wall, passNow, time.Minute, old), wall.Add(30*time.Second); !got.Equal(want) { |
| + t.Fatalf("an operation thirty seconds from its replay cutoff: %s, want %s", got, want) |
| + } |
| +} |
diff --git a/internal/console/billingquantity.go b/internal/console/billingquantity.go| index 15906c031..26513d2da 100644 |
| --- a/internal/console/billingquantity.go |
| +++ b/internal/console/billingquantity.go |
| @@ -418,6 +418,11 @@ var quantityOperationSendWindow = 2 * time.Minute |
| // lease past its end without waiting for it. |
| var quantityClock = time.Now |
| +// quantityWall is the wall clock a send deadline is fixed against. It is |
| +// the machine's own clock in production. A test replaces it to stand for a |
| +// sender that was suspended between taking its lease and sending. |
| +var quantityWall = time.Now |
| + |
| // Past this age an unacknowledged operation is never re-sent: the provider |
| // has forgotten its idempotency key, so a send would be a new request, not |
| // a replay. |
| @@ -1046,6 +1051,12 @@ func (s *Server) sendQuantityOperation(ctx context.Context, state billingState, |
| return quantityStep{}, err |
| } |
| claimed := quantityClock() |
| + // The wall clock, read BEFORE the lease is claimed. The send deadline |
| + // below is fixed from this reading and the lease's own end, as one |
| + // absolute instant. It is never recomputed from a later reading of any |
| + // clock: a sender suspended after its claim wakes to the deadline it |
| + // had, not to a fresh one, however long it slept. |
| + wallAtClaim := quantityWall().Round(0) |
| until, ok, err := s.store.claimQuantitySend(op.AccountID, op.Key, token, claimed) |
| if err != nil { |
| return quantityStep{}, err |
| @@ -1059,8 +1070,11 @@ func (s *Server) sendQuantityOperation(ctx context.Context, state billingState, |
| // transport makes that its last check, after the connection is |
| // established (billingstripe, transport.go), so a sender frozen here |
| // cannot send late on a connection of its own making. |
| - window := until.Add(-billingClockTolerance).Sub(claimed) |
| - sendCtx, cancel := context.WithTimeout(billingstripe.WithSendBy(ctx, time.Now().Add(window)), window) |
| + if hook := b.quantityAfterClaim; hook != nil { |
| + hook() |
| + } |
| + sendBy := quantitySendBy(wallAtClaim, claimed, until, time.Unix(op.CreatedAt, 0)) |
| + sendCtx, cancel := context.WithDeadline(billingstripe.WithSendBy(ctx, sendBy), sendBy) |
| updated, err := b.client.UpdateQuantityWith(sendCtx, op.Item, op.Target, op.Behavior, time.Unix(op.CreatedAt, 0), op.Key) |
| cancel() |
| if err != nil { |
| @@ -1354,3 +1368,18 @@ func (s *Store) fileBillingReviewOnce(accountID int64, subscription, reason stri |
| } |
| return tx.Commit() |
| } |
| + |
| +// quantitySendBy is the absolute wall-clock instant after which a quantity |
| +// request must not be written. It is the earlier of two instants: the send |
| +// lease's end less the clock tolerance, and the operation's replay cutoff |
| +// (past it the provider may have forgotten the key, and the same request |
| +// would run as new). Both are carried from the lease clock to the wall |
| +// clock through one pair of readings taken together before the lease was |
| +// claimed: wallAtClaim on the wall, claimed on the lease clock. |
| +func quantitySendBy(wallAtClaim, claimed, leaseUntil, operationCreated time.Time) time.Time { |
| + left := leaseUntil.Add(-billingClockTolerance).Sub(claimed) |
| + if cutoff := operationCreated.Add(billingIdempotencyReplayLimit).Sub(claimed); cutoff < left { |
| + left = cutoff |
| + } |
| + return wallAtClaim.Add(left) |
| +} |
diff --git a/internal/console/billingquantity_test.go b/internal/console/billingquantity_test.go| index 0b20e6b45..e6e9fe88b 100644 |
| --- a/internal/console/billingquantity_test.go |
| +++ b/internal/console/billingquantity_test.go |
| @@ -2548,3 +2548,63 @@ func TestQuantityYoungOperationWithAnOpenReviewIsNotSentAgain(t *testing.T) { |
| t.Fatalf("billed %d, want 11", state.Quantity) |
| } |
| } |
| + |
| +// A sender takes its send lease and is then suspended. Its request's |
| +// deadline was fixed, as one absolute instant, from a clock reading taken |
| +// before the lease was claimed. When it wakes the deadline has passed and |
| +// nothing is sent: the deadline is not built again from the clock it wakes |
| +// to. The operation stays outstanding and the next pass sends it. |
| +func TestQuantitySendDeadlineDoesNotMoveWhileTheSenderIsSuspended(t *testing.T) { |
| + fx, q, now := newQuantityFixture(t, billingProrationImmediate) |
| + if err := fx.st.AddMember(OpsAccountID, "eleventh@example.test", RoleMember, now.Add(time.Minute)); err != nil { |
| + t.Fatal(err) |
| + } |
| + // The suspension, without waiting it out: the wall reading taken at |
| + // the claim is as if the claim happened all but 30ms of the send |
| + // window ago, and the sender then sleeps past what is left. |
| + window := quantitySendLease - billingClockTolerance |
| + prevWall := quantityWall |
| + quantityWall = func() time.Time { return time.Now().Add(-(window - 30*time.Millisecond)) } |
| + t.Cleanup(func() { quantityWall = prevWall }) |
| + slept := 0 |
| + fx.srv.billing.quantityAfterClaim = func() { |
| + slept++ |
| + time.Sleep(90 * time.Millisecond) |
| + } |
| + if err := fx.srv.billingQuantityPass(context.Background(), time.Now().UTC()); err != nil { |
| + t.Logf("the pass with a suspended sender: %v", err) |
| + } |
| + if slept == 0 { |
| + t.Fatal("fixture: the sender never claimed its lease") |
| + } |
| + if len(q.posts) != 0 { |
| + t.Fatalf("a sender that woke after its deadline sent: %+v", q.posts) |
| + } |
| + if len(pendingAdditions(t, fx)) != 1 { |
| + t.Fatal("the addition is no longer pending") |
| + } |
| + quantityWall = prevWall |
| + fx.srv.billing.quantityAfterClaim = nil |
| + if err := fx.srv.billingQuantityPass(context.Background(), time.Now().UTC()); err != nil { |
| + t.Fatal(err) |
| + } |
| + if len(q.posts) != 1 || q.posts[0]["quantity"] != "11" { |
| + t.Fatalf("not sent afterwards: %+v", q.posts) |
| + } |
| +} |
| + |
| +// The deadline is the earlier of the lease's end less the tolerance and the |
| +// operation's replay cutoff, carried to the wall clock through the pair of |
| +// readings taken before the claim. |
| +func TestQuantitySendByIsTheEarlierOfTheLeaseAndTheReplayCutoff(t *testing.T) { |
| + wall := time.Date(2026, 10, 1, 9, 0, 0, 0, time.UTC) |
| + claimed := time.Date(2026, 10, 1, 9, 0, 2, 0, time.UTC) // the lease clock, two seconds off the wall |
| + until := claimed.Add(quantitySendLease) |
| + if got, want := quantitySendBy(wall, claimed, until, claimed.Add(-time.Hour)), wall.Add(quantitySendLease-billingClockTolerance); !got.Equal(want) { |
| + t.Fatalf("a young operation: %s, want the lease end less the tolerance %s", got, want) |
| + } |
| + old := claimed.Add(-billingIdempotencyReplayLimit).Add(time.Minute) |
| + if got, want := quantitySendBy(wall, claimed, until, old), wall.Add(time.Minute); !got.Equal(want) { |
| + t.Fatalf("an operation one minute from its replay cutoff: %s, want %s", got, want) |
| + } |
| +} |
diff --git a/internal/console/billingservice.go b/internal/console/billingservice.go| index 98818dff0..ba1359bf4 100644 |
| --- a/internal/console/billingservice.go |
| +++ b/internal/console/billingservice.go |
| @@ -58,6 +58,10 @@ type billingRuntime struct { |
| // before it sends the provider update (billingquantity.go). |
| quantityBeforeBegin func() |
| quantityBeforeSend func() |
| + // quantityAfterClaim is a test seam only: it runs after the send lease |
| + // is claimed and before the request's deadline is used, where a |
| + // suspended sender would sleep. |
| + quantityAfterClaim func() |
| // quantityAfterSend runs after the provider accepted an update and the |
| // operation was confirmed, before the worker's own reconcile. |
| quantityAfterSend func() |