Rewrite test_followup_queries_are_capped_and_serve_the_neediest_first
(the test that pinned the residual 32-project limitation) to assert both
starved projects are served in neediest-first order, and add
test_every_starved_project_is_served_past_the_old_refill_cap: 39 starved
projects with the real constants, all 39 must get their scoped follow-up.
The latter fails with the old starved[:32] slice (7 projects missing).
The refill after a saturated global assigned window was cut to the first
32 starved projects in deterministic neediest-first order. The recovery
is re-derived from state.db on every sidebar build, so the same 32 won
every time and every later project stayed unreachable on each rebuild —
the exact starvation the pass exists to repair (greptile P1 on #6659).
Drop the project-count cap. The follow-up's work is bounded by
construction instead: it only fires when the global window came back
saturated, each scoped query is limited to one project's remaining
budget, and the sum of those budgets across every starved project is at
most effective_limit * len(projects) <= PROJECT_ASSIGNED_CLI_SCAN_CEILING.
A fixed cap bounded nothing that the per-project limits did not already
bound; it only decided which projects were permanently left out.
The rebase onto e88aff438 put upstream's subagent-parent recovery walk
(8686528b1, supersedes #7031) and this PR's CANDIDATE_WINDOW_MULTIPLIERS
re-widening loop in the same tail of read_importable_agent_session_rows.
The walk anchors a frozen orchestrator by re-reading the projection of the
candidate window, so it has to see the window that ACTUALLY executed, not
the first pass.
Nothing pinned that. Feeding the walk a stale first-window projection keeps
every test in tests/test_subagent_parent_in_import_window.py green, because
those dbs never trigger the second window.
Add the missing case: two 12-segment compression lineages own the 24 newest
raw rows and collapse to 2 logical conversations, so a limit=3 request
consumes the 8x window and under-delivers; the hot subagent leaf and its
quiet parent are only reachable once the window re-widens. Plus a negative
control that pins the 8x window really does stop at the two lineages, so the
recovery is attributable to the widened window.
Verified with teeth: removing the walk fails the new test and 4 upstream
ones; feeding the walk the stale first window fails the new test alone.
The starvation fix is only acceptable if its cost is bounded, so assert the cost
directly: one GROUP BY probe plus exactly one project-scoped query per starved
project, each carrying only that project's remaining budget, with the recovered
total under PROJECT_ASSIGNED_CLI_SCAN_CEILING.
The per-project budget was applied to the rows of ONE global recency-ordered
query, so it was not a per-project bound: when a single project owns the whole
`limit * len(projects)` window, every other project's assigned conversations are
never considered and their chips show nothing (greptile P1 on #6659, reproduced
at the real 200 as well as scaled down).
Two tiers now:
* the global query still runs first and is the only cost a profile whose assigned
history fits in the window pays;
* when it comes back SATURATED, one GROUP BY over sessions.project_id says which
projects still own undelivered rows, and each of those gets its own
project-scoped query with its own remaining budget. `project_ids` narrows the
lineage CTE's seed, so a per-project query is cheaper than the global one and
still returns whole compression lineages.
Cost stays bounded: worst case per build is 1 global query + 1 GROUP BY probe +
PROJECT_ASSIGNED_CLI_REFILL_PROJECTS (32) project-scoped queries, each bounded to
one project's remaining budget. The scan ceiling is now shared equally between
projects (ceiling // len(projects)) rather than first-come, so the recovered
payload stays under the ceiling without letting the loudest project take it all.
profile_scoped_project_ids() duplicated the body of the canonical
api/profiles.py::_profiles_match, whose docstring says it was relocated there
precisely so callers stop duplicating it. Call it instead.
Behaviour-identical: both spellings agree on all twelve None / '' / 'default' /
renamed-root / foreign-profile combinations, which the new parametrised test
asserts cell for cell against _profiles_match itself. It is a drift guard, not
a regression test — swapping the call back to a raw
`p.get('profile') == active` comparison reddens seven of its twelve cells.
_merge_state_row overwrote an already-represented lineage unconditionally, and
recovery pass 2 (the unassigned refill) merges into the same lineage-keyed
table pass 1 fills. A row already carrying a RESOLVED project_id could
therefore be replaced by a bare unassigned projection — deleting the chip this
PR exists to restore and swapping the row's identity from the lineage tip back
to its root.
Reachable wherever the SQL project_assignment filter and the Python lineage
projection disagree about where a compression lineage starts. They differ today
on a blank `source`: _is_continuation_session treats an empty/whitespace source
as "unknown, same conversation", while the lineage CTE compares
LOWER(TRIM(parent.source)) = LOWER(TRIM(child.source)) and so will not walk
from an assigned tip up to a blank-source root. The mixed first pass collapses
the pair into one assigned row; pass 2 then hands the root back as its own
"unassigned" conversation and clobbers it.
The merge is now one-way: overwrite only when the incoming row ADDS a resolved
assignment, or when the row on file has none to lose. Fixing the merge rather
than each possible divergence — it is the one place where "already resolved" is
knowable locally, and pass 2 by construction has less assignment context than
pass 1.
tests/test_issue2841_show_cron_sessions_toggle.py reads this call site as
SOURCE TEXT (`'("cron", "webhook", "kanban") if source_filter is None'`), so
routing it through the new BACKGROUND_CLI_SOURCES constant turned a
pre-existing test in the maintainer's own shard gate red for a
behaviour-neutral refactor.
Put the literal back at that one call site. BACKGROUND_CLI_SOURCES stays where
the kanban-chip fix actually needs it (the background-source check in
_interactive_row_project_id, and interactive_excluded for both recovery
passes). A new test builds the expected literal FROM the constant and asserts
it appears verbatim in api/models.py, so the two spellings cannot drift
without failing here rather than in an unrelated file.
Recovery pass 2 was gated on `first_pass_count >= unassigned_target`, reading a
short first pass as "the database has nothing left". It can also mean the mixed
pass ran out of RAW candidate window: compression segments are spent from it
before the logical slice, so a lineage-heavy profile gets fewer conversations
than it asked for and the unassigned quota stays short. Also run the refill when
assigned conversations inside the window displaced unassigned candidates, which
is the case a narrower unassigned-only query can actually repair. A profile with
no assignments and a small state.db still issues no extra query.
While here, spend the per-project assigned budget through `_merge_state_row`'s
return value instead of pre-incrementing, so the budget counts logical
conversations the way PROJECT_ASSIGNED_CLI_LIMIT documents, and seed it from the
assigned rows already inside the window so the bound stays a total rather than
one bound stacked on another. That accounting is behaviour-neutral today (the
assigned query is newest-first, so window rows are always the first rows it
returns) — it makes the documented contract hold by construction rather than by
that ordering accident, and removes an unused return value.
`_interactive_row_project_id` resolved state.db assignments for anything that
was not cron or webhook, but the interactive projection loop is also what a
`source_filter='kanban'` scan walks. The same kanban row therefore reported two
different chips depending on the caller: None in the default sidebar (its own
bounded second pass, upstream behaviour) and a resolved project_id under
?source=kanban.
Route every source that owns a bounded project-chip pass back through
`_state_row_project_id`, and name that set once (BACKGROUND_CLI_SOURCES) so the
exclusion list and the chip decision cannot drift apart.
`?all_profiles=1` passed `project_assigned_limit=False`, on the premise that
its `visible_session_limit=None` first pass already returns every conversation.
It does not: `_load_cli_sessions_uncached` maps `visible_session_limit is None`
to CLI_VISIBLE_SESSION_LIMIT for the interactive pass, so that view truncates
by recency exactly like the single-profile one. Only cron/webhook/kanban limits
reach the reader as `limit=` directly, where None does mean unbounded.
Net effect of the old value: in the "All profiles" sidebar a project-assigned
CLI session older than the newest 20 interactive conversations was missing from
/api/sessions while its project chip still claimed it — the #6659 bug itself,
in the one view that shows every profile at once. Recovery pass 2 (the
unassigned refill) still ran there, so the two passes disagreed as well.
Pass PROJECT_ASSIGNED_CLI_LIMIT instead of None so the per-project bound and
the PROJECT_ASSIGNED_CLI_SCAN_CEILING guard apply per profile, rather than
issuing an unbounded assigned scan for every profile on the box.
Mutation-checked the #6659 suite and found two branches no test held:
* the model recovery pass's per-project accounting. Its query asks for
PROJECT_ASSIGNED_CLI_LIMIT * len(projects) conversations, so on a
single-project profile query_limit == the bound and the LIMIT alone
produces the right answer. Deleting the per-project counter kept all
21 tests green. Only a skewed multi-project profile (250 in one
project, 5 in another, scan window 400) exercises it.
* schemas with no project_id column, end to end through the model.
_state_db_supports_project_ids is asserted directly because dropping
the probe is behaviour-neutral -- the assigned filter already returns
nothing for such a schema -- so it is a query-budget guard, and the
contract worth pinning is the probe's answer plus unchanged output.
Both new tests fail against a tree with the corresponding branch removed.
Each behavioral test fails on the pre-fix tree and passes after:
- 1,000 assigned rows arriving as imported sidecars -> 1,000 kept (finding 1)
- 500 assigned rows over two projects -> per-project bound, so >200 total survive
- 3 assigned + 20 unassigned -> 17 unassigned, at both the route cap and the
model window, including the logical-conversation count after lineage dedup
(finding 2)
- a deleted and a cross-profile project_id -> 'project-ghost' surfaced instead of
None, and an old ghost row resurrected as an unreachable chip-only row
(finding 3)
- a project assignment on a newer empty continuation -> dropped, both at the
projection level and end-to-end through the sidebar (finding 4)
- twelve ten-segment chains, limit=10 -> 8 conversations (greptile P1)
- 250 assigned in one project -> the newest 200, oldest dropped, never a newer one
- 'assigned'/'unassigned' partition the conversations exactly, including a
lineage assigned only on its tip, and on a schema with no project_id column
- master's kanban second pass still projects rows (signature regression guard)
Letting assigned sessions past the *recent* cap was right; letting them past
*all* bounds was not. Three linked defects from review of #6659:
1. The assigned list was effectively unbounded. models.py's 200 cap only applied
to state.db's own pass; imported CLI sidecars from all_sessions() never pass
through it and _cap_recent_cli_sessions() retained every assigned row (1,000
assigned rows -> 1,000 returned). The FINAL MERGED payload is the only place
that sees every source of assigned rows, so bound it there - per project, so
one busy project cannot evict another's history (the >200-assigned case
greptile flagged), and counting logical conversations, not raw rows.
2. Assigned rows spent the unassigned quota: 3 assigned + 20 unassigned yielded
17 unassigned, and the model fetched 20 rows total up front so bumping only
the route counter could not refill. Count the two separately in the route cap
and add a bounded unassigned refill pass, keyed on the lineage, so 20 unique
unassigned logical conversations survive sidecar/lineage dedup. The refill
only runs when the window was actually saturated AND came up short.
3. A deleted or cross-profile project_id hid the session outright: it was
surfaced unchecked, so the row left "Unassigned" and its overflow became
default_hidden with no chip left to reveal it. Resolve non-system ids against
profile-scoped load_projects() and coerce a miss to None BEFORE either cap -
the mirror of the bug this PR fixes.
read_importable_agent_session_rows() grows a `project_assignment` filter
('assigned'/'unassigned') replacing the require_project_id boolean. Both arms
key on the whole compression lineage and are exact complements, so neither
budget can double-count a lineage.
_state_row_project_id() keeps its (sid, source) signature: master's kanban pass
calls it that way, and giving it a row-dict signature turned that whole pass
into a swallowed TypeError that dropped every kanban row.
read_importable_agent_session_rows()'s `limit` is a bound on LOGICAL
conversations (the slice runs after lineage projection), but the bounded
candidate CTE spends its raw window on compression segments and rows the
post-projection visibility filters drop. A lineage-heavy profile therefore got
back far fewer conversations than it asked for, and silently: with twelve
ten-segment chains, limit=20 returned 8.
Retry once with a wider window when the first one was fully consumed AND still
under-delivered. The common case is unchanged - the first window is still 8x the
requested limit and the retry never runs when the db simply has nothing more to
give - so the #2628 capped-scan guarantee holds.
compression_tip() resolves a project assignment across the whole lineage, so it
can find one recorded on a newer EMPTY continuation. When the root is still the
freshest *importable* segment, the `tip is row` early return appended the root
without applying that resolved id and the assignment was silently dropped.
Copy the root and apply lineage_project_id before appending, matching what the
merged-tip path below already does.
Two independent 20-session caps hid older project-assigned CLI sessions:
models.py only queried the 20 most recent, and routes.py capped the API
payload again. Fixing the query alone was not enough.
- agent_sessions.py: optional project_id filter; propagate the freshest
project_id along a compression lineage (compression_tip now returns a
3-tuple) so a Project set only on the tip is not lost
- models.py: second bounded query for older project-assigned sessions;
skipped when the schema has no project_id; dedupe by id/lineage
- routes.py: keep the 20 recent, drop unassigned overflow, preserve
assigned overflow as default_hidden (revealed by the project chip)
Adds regression tests for the limit, the root/tip lineage cases and the
route cap.
A persisted message_count is only authoritative when the write that
produced the messages array also vouched for it. Sidecars written by an
older writer (recovery materialization copied the denormalized state.db
count over real rows) could carry a count stale against the array beside
it, and the prefix shortcut then reads a real shrink as a growth and
skips the #1558 backup.
save() now stamps _mc_v alongside message_count, and _prefix_message_count
refuses any count without the current marker: those files get the full
parse, exactly as on the legacy path. One save re-marks the file, so the
fast path resumes for steady state.
Regression covers the unmarked stale-count shape and the re-marking
transition; mutation-checked by removing the gate (the unmarked fixture
then skips the backup, as the review's reproduction predicted).
The #1558 backup safeguard in Session.save() compares the on-disk message
count with the incoming one and writes <sid>.json.bak only when the save would
shrink the array. Sound -- but it obtained the on-disk count by
json.loads(self.path.read_text()), the entire file, on EVERY save, including
the 99.9% of saves that grow the conversation and never back anything up.
Measured in production (Mac docker stack), in the running container:
sessions/fa3bca34a0c6.json 203,439,398 bytes / 266,940 messages
read_text 17,453 ms
json.loads 2,924 ms
TOTAL to obtain one integer: 20,377 ms <- paid on every save
That was the 39,357 ms /api/session request stuck at t6_after_json_write,
after the read-side re-parse had already been fixed. It scales linearly with
the file, and the file grows for as long as the conversation does.
The decision below needs ONE integer. save() already writes that integer into
the metadata prefix, before `messages`, as `message_count` -- exactly so
readers can have it without parsing the body, which is how
load_metadata_only() and the sidebar freshness check already work. So save()
now reads THAT, through the same bounded 64 KiB prefix reader, and falls back
to the full read + parse whenever the prefix does not carry a usable count: a
legacy pre-#5854 sidecar whose count serialized after the scenes, a file with
no top-level `messages` key at all, a corrupt or truncated prefix, metadata
alone that overflows the budget, or an unreadable path. Fail-open is the
contract -- never "assume no shrink". The full text is still read for one
thing only: the .bak body, when a shrink is actually detected.
An in-memory stat identity was tried first and is NOT sufficient, so it is
gone. The first revision of this branch stamped the (inode, size, mtime_ns) of
the bytes it wrote and trusted the remembered count while that tuple matched,
on the reasoning that "stat says nothing touched it, so the array is the one I
wrote". Review (nesquena-hermes, 2026-09-21) reproduced the hole: a stat tuple
is not a content identity. A same-length in-place rewrite inside one mtime tick
keeps inode, size AND mtime_ns -- ext4 stamps mtime from a coarse clock, so two
writes in the same tick share one mtime_ns -- and the next shrinking save then
reads a stale count, concludes it is growing, overwrites the longer transcript
and writes no .bak. That is the #1558 data-loss shape the safeguard exists to
prevent, reachable on this branch. A count read from the file's own bytes
cannot be fooled that way: any rewrite of the bytes rewrites the count.
So _disk_identity() is removed along with the identity fields on Session, the
stamp after the write, and the seeding in load(). _prefix_message_count() is
the replacement: bounded prefix -> the number the writer itself recorded.
One trap survives the redesign and is pinned. load() de-duplicates adjacent
partials and immediately saves the shorter transcript (#2592), and that save
must produce a .bak because it shrinks the array on purpose. The count that
decides it is the one the writer stamped on the file -- the ON-DISK length it
wrote -- not the post-collapse length of the object. Read the object's own
length instead and the #2592 backup disappears silently.
session_recovery._msg_count() pays the same cost at boot and is deliberately
left alone. Its "torn file -> -1" contract is what makes recovery restore a
.bak over a truncated live file, and no bounded read can prove a file is not
truncated. A boot pays it once; a save paid it every time.
Tests: 9 in tests/test_save_count_without_full_parse.py. The review's
reproduction is the last one -- a same-inode, same-byte-length, same-mtime_ns
in-place rewrite of 2 -> 5 messages, then a save of 3, which must leave the
5-message array recoverable. It fails against the removed stat-identity design
(re-verified by restoring that revision: no .bak is written and the
5-message transcript is lost) and passes here. Mutation-checked the same way:
recording the post-collapse length fails exactly the #2592 test. 783 tests
pass across the 41 files that touch this surface (save/load, metadata-only,
collapse, .bak recovery, sidecar reconciliation); ruff is clean on the diff.
The mirror image of the sidecar over-close already fixed here: the sibling
reject-before-read sites still armed `arm_connection_close()` unconditionally,
so a request with NO declared body was answered with `Connection: close` and
the client's pipelined follow-up was dropped -- a healthy pooled connection
killed for no framing reason. Reproduced on the wire, pipelined down one
socket against the production handler (`responses seen: 1` at each site):
POST /api/session/new off-session auth -> 401 + Connection: close
POST /api/session/new bad Origin -> 403 + Connection: close
POST /api/session/new no CSRF token -> 403 + Connection: close
POST /api/csp-report limiter tripped -> 204 + Connection: close
POST /api/health/restart success -> 200 + Connection: close
POST /api/process-complete-ack -> 410 + Connection: close
POST /api/upload no boundary -> 400 + Connection: close
Each now routes through `arm_connection_close_if_body_pending()` (the helper
the sidecar path already used), so the framing decides in both directions:
body-bearing or unreadable framing still closes, an absent or agreeing-zero
Content-Length keeps keep-alive. `check_auth_or_close()` cannot swap the helper
in directly -- it has to arm before `check_auth()` writes its 401/302 and undo
that on success -- so it gates the arming on `request_declares_body()` instead
and keeps the success-path restore.
`/api/health/restart` is armed once, ahead of the outcome, so all four results
(completed / in_progress / busy / error) follow the framing rather than the
result; the success path was closing on every call the WebUI makes.
Only the boundary check in the upload preflight can reject a bodyless request:
every other raise there (any Transfer-Encoding, an unreadable or conflicting
length, an over-cap length) already declares a body and still closes.
Five tests pinned the unconditional close on stubs that declared no body --
i.e. they pinned the bug. Each is re-grounded on real body-bearing framing
(Content-Length, chunked, and an unreadable length) so the close half stays
pinned, with a bodyless sibling asserting keep-alive. New real-socket
regressions cover all seven sites in both directions.
`identity` was exempt as a coding that "frames nothing", so a request
carrying it read back as body-less however many payload bytes followed.
It frames nothing either: RFC 9112 6.3 says a request whose final
transfer coding is not `chunked` has a body length no recipient can
determine, RFC 7230 removed `identity` from the transfer codings
outright, and `http.server` decodes none of them (`rfile` is the raw
socket). Every present value has to reject-and-close.
Reproduced on 8c326003b, pipelined down one socket:
GET /api/extensions/probe/sidecar/ping `Transfer-Encoding: identity`
{"stale": true}
-> HTTP/1.1 403 Forbidden (no Connection: close)
-> HTTP/1.1 400 Bad request syntax
('{"stale": true}GET /api/health/agent HTTP/1.1')
POST /api/upload (and /upload/extract, /workspace/upload, /transcribe)
-> HTTP/1.1 400 Bad Request ("No file field in request", no close)
-> HTTP/1.1 400 Bad request syntax ('--x')
Deletes the `_UNFRAMED_TRANSFER_CODINGS` allowlist and the fold-safety
test that existed only to guard its membership check; `_BLANK_TRANSFER_CODING`
stays, since a present-but-empty header still needs a named, non-falsy
report. The identity rows of the framing tables and of
`test_bodyless_framing_keeps_the_pooled_socket_alive` move to the
closes-the-socket side, with real-socket coverage on the sidecar path and
all four upload endpoints. The keep-alive half stays pinned by the cases
that genuinely have no body: no framing header at all, `Content-Length: 0`,
an OWS-padded zero, two agreeing zeroes.
Swept the rest of the coding surface for the same class: every case
spelling, OWS and non-OWS padding, `identity, chunked`, `chunked, identity`,
`identity,identity`, a lone comma, an unknown coding, duplicated headers,
and a coding beside both a real and a zero Content-Length.
`_declared_content_lengths()` parsed with `int()`, which is far looser than RFC
9110's `Content-Length = 1*DIGIT`: it accepts a leading sign, PEP 515
underscores and non-ASCII digits, and `str.strip()` ahead of it also erased
non-ASCII whitespace. Every such spelling of ZERO therefore parsed to an honest
`0`, took the "every declared length agrees on zero" keep-alive branch, and left
the payload in rfile for the server to parse as the next request line -- the
same defect the blank value had, one layer down. Reproduced live at the previous
head, pipelined down one socket:
GET /api/extensions/probe/sidecar/ping `Content-Length: +0` {"stale": true}
-> HTTP/1.1 403 Forbidden (no Connection: close)
-> HTTP/1.1 400 Bad request syntax
('{"stale": true}GET /api/health/agent HTTP/1.1')
byte-for-byte the trace greptile filed the P1 for, and identical for `-0`,
`0_0`, `+00`, `0_0_0`, `+0_0`, `\xa00`, `0\xa0` and `\x850`. On /api/upload the
same values slipped `unreadable_content_length()` into `int('0_0')` == 0 and
answered 400 keep-alive then `400 Bad request syntax ('--x')`.
A sign was reported safe last round because it was probed as `+15`, where a
wrong parse closes anyway; only the zero-valued spellings expose it.
The parse is now the grammar: an unsigned run of ASCII digits, or the framing is
unreadable and a body is assumed pending. The `int()` guard stays -- a digit run
longer than `sys.get_int_max_str_digits()` (4300) raises ValueError too, and a
raise on a rejection path becomes a spurious 500.
`Transfer-Encoding` gets the same audit. Trimming is now SP/HTAB only, RFC 9110
OWS and nothing else, because `str.strip()` erased U+00A0/U+0085 there too:
`Transfer-Encoding: \xa0identity` -- a spelling no token grammar allows -- read
back as the `identity` token, so it counted as unframed and poisoned the socket
with the same 403-then-400 trace (411 and close on uploads). Both characters
reach the wire, since a request line decodes as latin-1. Membership in
`_UNFRAMED_TRANSFER_CODINGS` is tested through `str.lower()`, which maps some
non-ASCII characters ONTO ASCII ones (U+212A -> `k`), so the set is now
documented as ASCII-token-only and a test scans every codepoint to prove no
non-ASCII spelling can reach it.
Non-ASCII digits are unreadable by the same rule and pinned at the helper level:
U+0660/U+FF10 cannot cross an HTTP/1.1 header line (latin-1 leaves only the
U+00B2/B3/B9 superscripts, which `int()` already rejects), but the helpers take
any headers mapping and enforce the grammar rather than trusting the transport.
The no-body contract is unchanged and pinned in both directions on the wire and
at the helper: no length header, `0`, `00`, an OWS-padded zero, two AGREEING
zeroes, and `identity` in any ASCII case still keep the pooled connection alive.
`Content-Length:` with nothing after the colon reads back as '' from
`Message.get_all()`, and `_declared_content_lengths()` SKIPPED the empty value.
The result was "no length declared", so `request_declares_body()` reported no
pending body and the sidecar provenance rejection answered 403 with keep-alive
intact -- leaving the payload in rfile for the server to parse as the next
request line. Reproduced live, pipelined down one socket:
GET /api/extensions/probe/sidecar/ping `Content-Length:` {"stale": true}
-> HTTP/1.1 403 Forbidden (no Connection: close)
-> HTTP/1.1 400 Bad request syntax
('{"stale": true}GET /api/health/agent HTTP/1.1')
A blank value cannot prove a body's absence, so it now joins `banana` and two
disagreeing lengths as unreadable framing: bytes are assumed pending and the
connection closes. Absence is something only the wire can say, and an absent
header already yields no values at all.
Three adjacent holes of the same class were found by probing and are fixed by
the same rule (each reproduced live with the same bad-syntax 400):
* whitespace-only `Content-Length: ` -- `Message` collapses it to the same '',
as it does a duplicated blank or a blank paired with a real length.
* the four multipart upload handlers: a blank length slipped past
`unreadable_content_length()`, reached `int('' or 0)` == 0, and answered 400
"No file field in request" with the payload still queued. Now 400 "Invalid
Content-Length" with the connection closed.
* blank `Transfer-Encoding:` -- '' sat in `_UNFRAMED_TRANSFER_CODINGS` beside
`identity`, so a header naming no coding read as "nothing to frame". A blank
coding is undecodable, not unframed; uploads answer 411 and close.
The no-body contract is unchanged and pinned in both directions: no length
header, a padded zero, two AGREEING zeroes and `identity` all still keep the
pooled connection alive.
Adversarially probing the re-gated head found the same poisoning reachable by
DUPLICATING a framing header instead of omitting one. `Message.get()` returns
only the first occurrence, so `Content-Length: 0` sent ahead of the real length
made the body invisible to every reader: nothing was drained and the real bytes
were parsed as the next request line. Reproduced live on the otherwise-fixed
head, over one pooled socket:
HTTP/1.1 403 Forbidden <- sidecar provenance reject
HTTP/1.1 400 Bad request syntax ('{"stale": true}GET /api/health/agent ...')
HTTP/1.1 400 Bad Request <- /api/upload reject
HTTP/1.1 400 Bad request syntax ('--x')
api/helpers.py now reads framing through `_framing_header_values()`, which
returns every value the wire carried. `request_declares_body()` treats
disagreeing or unparseable lengths as a declared body (they cannot prove the
body's absence), and `unsupported_transfer_encoding()` inspects every declared
coding so `identity` cannot mask a `chunked` behind it. Two agreeing zeroes are
still no body -- repetition alone is not a conflict.
New `unreadable_content_length()` gates the multipart preflight: with no honest
byte count there is nothing to drain, so all four handlers refuse with 400
"Invalid Content-Length" before touching rfile, connection armed for close.
13 regressions, each verified red without this commit: the helper predicates on
a real email.message.Message, the production sidecar 403, all four multipart
handlers, and five pipelined real-socket tests proving the follow-up request is
never answered off a poisoned socket.
Two framing gaps the re-gate reproduced on the production handlers, both the
same class: something other than the wire was used to decide whether unread
body bytes are queued.
1. api/routes.py — the sidecar provenance 403 gated its close on
`read_request_body`, a static per-method flag. That is wrong in both
directions: a GET carrying `Content-Length`/`Transfer-Encoding` was answered
with keep-alive and its body then parsed as the next request line
(`501 Unsupported method ('{}GET')`), while a body-less DELETE/PUT/PATCH had
a healthy pooled connection closed for nothing.
2. api/upload.py — all four multipart handlers read framing from
`Content-Length` alone, so a chunked upload (which has none) was treated as
length zero, matched no parts and was rejected 400 "No file field in
request" with keep-alive intact. Wire trace before the fix:
HTTP/1.1 400 Bad Request <- the upload rejection
HTTP/1.1 400 Bad request syntax ('54') <- our own chunk-size line
New primitive in api/helpers.py: `request_declares_body()` (Content-Length
present and non-zero, or any Transfer-Encoding other than identity; an
unparseable length counts as present, since it cannot prove absence) plus
`arm_connection_close_if_body_pending()` and `unsupported_transfer_encoding()`.
api/upload.py now routes all four handlers through one preflight,
`_read_multipart_or_reject()`, which refuses an undecodable transfer coding
with 411 before touching rfile — http.server never decodes chunked framing, so
that request can only be rejected — and keeps every previous status and message
(413 oversize, 400 invalid Content-Length/boundary) with its close.
Regressions: chunked reject that closes and a body-less reject that does not,
at all four upload handlers and at the sidecar 403, plus two pipelined
real-socket tests against the running server proving the follow-up request is
never answered off a poisoned socket.
advertise_connection_close() guarded its two attribute reads but not the
dedup scan over _headers_buffer. A stub whose buffer is truthy and
non-iterable (a bare Mock) raised TypeError from inside end_headers(),
which _handle_write's generic except would swallow into a spurious 500 --
the same failure class as the missing close_connection guard.
Replaces the inline `handler.close_connection = True` at each reject-before-read
site with api.helpers.arm_connection_close(), the shared mechanism the review
asked for. Behaviour is unchanged at every real handler; the helper adds the
same stub tolerance the read side just got, so a handler that refuses the
attribute can never turn a 4xx into a 500.
Sites: _check_csrf (origin + token), the sidecar provenance 403 (body-reading
calls only), the rate-limited CSP report, /api/health/restart, the deprecated
process-complete-ack 410, and all four upload handlers' pre-read
length/boundary rejections.
The first cut of the close advertisement had two defects, both caught by CI
(9 red jobs) and by the deep-review gate's own MUST-FIX 1 wording.
1. Dedup was keyed off a private `_close_advertised` flag that end_headers()
set itself. Every SSE endpoint already sends its own `Connection: close`,
and BaseHTTPRequestHandler.send_header flips close_connection when it does
— so end_headers() appended a SECOND copy to every stream response.
Dedup now derives from the pending header buffer, which sees those
site-level headers too.
2. `self.close_connection` was read unguarded. The attribute only exists once
a request line has been parsed, so the many suite handlers built with
`Handler.__new__(Handler)` raised AttributeError instead of emitting
headers — and in _handle_write that AttributeError became a spurious 500.
Both reads are now defensive, and check_auth_or_close() no longer invents
the attribute on a handler that never had it (which would have left a bogus
keep-alive verdict behind).
The mechanism moves to api/helpers.advertise_connection_close(), which also
puts server.py back under the original 750-line guard, so the test's ceiling
bump to 760 is reverted rather than shipped.
Deep-review gate (2026-08-14) findings, all reproduced:
1. close_connection was never serialized: BaseHTTPRequestHandler does not
emit Connection: close from the flag, so pooled clients kept reusing the
socket and died with BrokenPipeError. Handler.end_headers() now emits
the header (deduped) whenever close_connection is armed.
2. check_auth_or_close armed the flag too late (after the 401/302 was
already flushed); it now arms before check_auth() and restores the
prior flag on success.
3. The sidecar provenance rejection closed body-less GETs too; the close
is now gated on read_request_body=True.
4. Remaining reject-before-read sites now close-and-advertise through the
same mechanism: rate-limited CSP reports (204), the deprecated
/api/process-complete-ack (410), /api/health/restart (all outcomes),
and the four upload handlers (413 + pre-read Content-Length/boundary
validation via _reject_before_read).
Tests: pooled-client regression against the production handler (reject
with a body, then follow-up on the same pooled connection succeeds and the
rejection carries Connection: close); unit coverage for the sidecar GET
keep-alive, CSP rate-limit, health/restart and upload oversize paths. The
server.py line guard is bumped to 760 with rationale: the deep-review
requires the emission in Handler.end_headers(), which cannot live outside
server.py.
Same class as the auth/CSRF close-on-reject: the sidecar proxy's
same-origin provenance check runs before read_body(), so an HTTP/1.1
connection reused after a 403 would parse the unread body as the next
request line. Flag close_connection on this path too.
Found by Greptile review of #6658; covered by a new unit test.
Move the close-on-reject decision into api.auth.check_auth_or_close so the
added lines land outside server.py, restoring it to 749 lines. Trim the
inline comment to a one-line docstring on the helper, per review feedback.
The rejected-write connection-close test now patches api.auth.check_auth
(the dependency the helper calls) instead of server.check_auth.
Read the metadata prefix in staged geometric reads so one oversized metadata field no longer forces a full re-parse of the whole sidecar.
Co-authored-by: rodrigogs <rodrigogs@users.noreply.github.com>
_syncSidebarAria set aria-expanded on the newly-active rail button without
unsetting the previous one, so chat -> kanban -> skills left three buttons all
claiming aria-expanded=true. A screen reader announced three open disclosures
when only one panel owns the sidebar.
Reset every rail button that carries the attribute before setting the active
one.
