Preserve usage report identity across collector retries

[stellasphere]

Problem and change

When an accepted usage request times out, the collector currently merges its retry with newly recorded usage. An accepted 10 frames followed by 5 new frames can therefore submit 15 again and count 25. For workspaces that explicitly advertise usage report v1, freeze the final resource payload and report ID before sending, retain the original through partial acknowledgments and retries, and atomically prepare/acknowledge persistent reports in the existing SQLite queue.

A supplied replacement credential can replay a retained report after live capability confirms its workspace, without changing its original body or ownership fingerprint. Memory and SQLite tests cover removal or retention of the old credential, an already-known replacement inserted earlier in the credential map, and unrelated-workspace isolation. A conflicted ownership basis keeps its original report while other owners continue; new usage for that same basis remains in the existing compacting raw queue. Unmarked persisted records and previously attempted legacy payloads keep legacy semantics, including when compacted with new usage. Transport markers are stripped, and the shared dependency-free helpers also serve the Redis offloader.

Validation

30 passing focused cases: 27 new helper/actual collector/SQLite cases plus 3 existing SQLite cases. These cover accepted timeout + new usage = 15, partial/malformed acknowledgments, immutable ownership across restart, memory/SQLite multi-owner progress, conservative legacy compaction, and SQLite transaction rollback after a failed prepared-report insert. The local runner substitutes unrelated model-startup modules because the full inference model runtime is unavailable; actual collector methods, helpers, SQLiteQueue, and SQLiteWrapper execute. HTTP is intercepted. Black and git diff --check pass. Full-target CI is not established by this local run.

Dependencies and rollout

Requires the receiver contract in https://github.com/roboflow/roboflow/pull/15282 and the matching Redis offloader change before collector activation. Deploy receiver first, then the compatible offloader pinned to this exact helper source, drain incompatible workers, then release/update inference producers. Old offloaders must not consume new marked raw records during rollout. Failed capability lookup retains unattempted usage; a report that has been attempted never downgrades or changes ownership.

This PR does not publish a package, deploy images, activate financial rollout, or decide PRO-402 funding-boundary policy. Durable raw storage retains its existing capacity behavior; prepared contents do not grow on retry, but no fixed-byte memory guarantee is claimed.

Review and project scope

Part of PRO-147
Part of PRO-98

This is the immutable producer/offloader delivery portion of the pricing revamp: retries must not double-count usage before canonical prepaid settlement can be safely used. It does not complete either issue’s full accounting or release scope.

Two dedicated context-isolated reviews approve the current head under the authorized Jarbas-outage substitute: correctness and simplicity. These are recorded approval verdicts in GitHub comments, not native or Jarbas approvals.

CodeQL alert 477 was independently traced to the existing deterministic API-key cache index and dismissed as a documented false positive; no source suppression or hash change was made.

[stellasphere]

Collector correctness review

Recommendation: REQUEST_CHANGES for roboflow/inference PR #2946 at addf8ee231ed555238241b2f16de2a6b1f33bf12 (base 5909dc7b0cfe75baab462082cb937a30481e555f). No repository edits or external publication performed.

P2: Resolve a replacement credential for retained same-workspace reports

Location: inference/usage_tracking/collector.py:795-801, _send_prepared_reports.

Prepared delivery is indexed by the original API-key hash. Sending can resolve only that exact hash through _hashed_api_keys; it silently skips a retained report when the original key is absent. After a process restarts with a rotated API key for the same workspace, normal new usage learns only the new hash. The original report remains permanently unsent even though an authorized replacement credential is available and its immutable report already contains the workspace identity. Keeping the revoked original key in memory also does not select the replacement key. This leaves usage pending without an automatic recovery route and fails the requested same-workspace credential-rotation delivery scenario.

Independent reproduction against the complete actual collector module and actual SQLite implementation: prepare 7 frames under old key, return HTTP 503, replace the live credential map with a new key for the same negotiated workspace, enqueue 3 new frames, then flush three times. The new 3 frames are accepted; the original 7-frame report remains byte-identical in SQLite and is never sent. This is a new-protocol recovery gap, not a claim that legacy telemetry supported key rotation. A fix should validate a candidate replacement credential against the retained report workspace and retry the identical report using that credential, without rewriting ownership or selecting an unrelated workspace key.

Verification

• Read repository AGENTS.md and the requested Review PR skill; inspected all five changed files and SQLite transaction implementation.
• Re-ran supplied actual-module test runner: 22 passed in 0.09s.
• Inspected the runner: it executes real collector, payload helper, SQLiteQueue, and SQLiteWrapper modules. It stubs startup configuration, plan lookup, Redis, logging, request decorators, TLS/header helpers, and thread startup. It is useful behavioral evidence, not a real production initialization or receiver integration test.
• Independently executed /private/tmp/collector-independent-review.py against full actual collector and SQLite classes (same startup dependency isolation; no AST-extracted replacement collector). Verified SQLite acknowledgement failure after receiver acceptance, database reopening, immutable duplicate replay, and eventual clean acknowledgement.
• Verified 105 raw unknown-credential rows do not indefinitely starve a later healthy-owner report: bounded paging and deferred-row reinsertion eventually delivered the healthy 3-frame report.
• Independently reproduced the same-workspace credential-rotation gap above.
• Source review and supplied tests cover immutable timeout retries, queue compaction, terminal/partial/malformed acknowledgements, old unmarked legacy migration, prepared-before-send atomic rollback, and new owner progress alongside conflicted retained reports. No other blocking correctness finding identified.
• Working tree remained clean. No public release, deployment, provider changes, or external comments made.

Replacement-credential boundary: only use credentials already supplied to the collector (for example through a new usage call, which populates _hashed_api_keys via _calculate_api_key_hash). Negotiate that candidate key and verify its returned workspace equals the retained report_workspace_id. Do not alter or require equality with the retained ownership fingerprint: a previously accepted original report must remain replayable after customer change, while an unaccepted original report must remain subject to the receiver's ownership-conflict result. The receiver decides acceptance using the original report ownership. This requires no guessed or externally discovered credential.

[stellasphere]

Collector correctness re-review

REQUEST_CHANGES at 496afa1d6fa54d0ffb834794fce47cc3793db583, verified as the published OPEN PR #2946 head via GitHub. Checkout clean.

P2: Scan supplied replacement credentials on both sides of the original key

inference/usage_tracking/collector.py:805-806: if candidate == original_key: break.

This stops the replacement search before any credentials first observed earlier than the original key. In a multi-key collector, a valid same-workspace key may already have been supplied before the subsequently revoked key. Supplying that valid key again does not change Python dictionary insertion order. The retained reports therefore continue using the revoked key forever even while new usage succeeds with the valid replacement.

Independent reproduction /private/tmp/collector-known-key-rotation-review.py loads the full actual collector and SQLite modules. It begins with supplied keys ordered replacement, old; prepares 7 frames under old and returns 401; then old capability lookup also fails with 401 while replacement capability identifies the original workspace. It supplies replacement again, enqueues 3 new frames, and flushes three times. The original 7-frame report remains retained and every attempt uses old; the replacement is never considered for it. Fix by skipping the original candidate without ending the search, while retaining workspace validation and immutable ownership/body.

Passed verification

• Re-ran actual module test runner: 28 passed in 0.09s.
• Original rotation reproduction now passes when replacement is newly inserted: old 7 plus new 3 accepted; immutable report unchanged except transport API key.
• Independent SQLite acknowledgement failure/restart replay passes.
• Independent 105-unknown-row healthy-owner fairness passes.
• Source confirms candidate discovery is limited to supplied keys, workspace matching excludes unrelated credentials, and report fingerprint/body are not rebound on customer changes.
• New author tests cover memory/SQLite, old key retained/absent, unrelated workspace, and changed current fingerprint. They only insert replacement after original, missing the case above.
• No source edits, external reviews, deployment, or release performed. This replaces the prior blocking finding only after the remaining search-boundary case is fixed.

[stellasphere]

Dedicated context-isolated correctness review — APPROVE exact head. Posted by the coordinating agent under the authorized Jarbas-outage substitute; this is not a native GitHub or Jarbas approval.

Collector final correctness review

APPROVE d3f8e74934827fdd2011874b16a265965c0af619 for roboflow/inference PR #2946. GitHub current OPEN head matches the reviewed local commit; working tree clean. Both previously reported credential recovery gaps are resolved. No outstanding significant correctness findings in this review scope.

The final production delta from 496afa1d6fa54d0ffb834794fce47cc3793db583 is the targeted break to continue change in supplied replacement-key scanning. It now considers earlier-known same-workspace keys while preserving every original report field and ownership fingerprint. It uses only supplied keys and verifies candidate workspace rather than rebinding report ownership.

Validation at this head:

• Actual-module author suite: 30 passed in 0.14s, including memory and SQLite earlier-known replacement coverage.
• /private/tmp/collector-independent-review-final.py: independent original rotation reproduction passes (7 retained + 3 new frames acknowledged; original body unchanged except transport API key); SQLite acknowledgement-failure/restart immutable replay and 105-row fairness also pass.
• Original earlier-known-key defect reproduction no longer reproduces: its assertion that old reports remain retained fails because the fixed collector successfully retires them.
• /private/tmp/collector-known-key-rotation-approved.py: independently verifies explicitly retained 7-frame report is replayed through an earlier-known replacement key after a failed delivery, alongside 3 fresh frames; total 10 acknowledged, original report unchanged except API key, queue drained. The initial transport is forced to 503 so the fixed eager credential selection cannot acknowledge the original before the recovery phase.
• Inspected the changed author tests and full credential-selection delta. Tests exercise actual collector/payload/SQLite modules with startup and transport stubs; they do not claim production initialization or real receiver integration.

No further receiver-body wire proof is required for this one-line fix: it changes candidate selection only, and independent checks verify source body, ID, timestamps, and fingerprint remain identical through replay. Existing receiver integration proof remains relevant; external deployment, package release, and provider actions remain outside this approval.

No source edits or external publication performed.

CodeQL follow-up:

CodeQL assessment at d3f8e74

Recommend per-alert false-positive dismissal with the evidence below, leaving hash behavior unchanged. No security state or source was changed during this review.

Exact alert: https://github.com/roboflow/inference/security/code-scanning/477
Query: py/weak-sensitive-data-hashing. Check run: 101912532217. Alert instance: refs/pull/2946/head, commit d3f8e74934827fdd2011874b16a265965c0af619, Python default analysis. Sink inference/usage_tracking/payload_helpers.py:249. The alert API identifies its sensitive-data source as collector.py:726, api_key = keys.get(key).

The source reaches collector.py:739 PlanDetails.get_api_key_plan, which computes sha256_hash(api_key, length=-1) at plan_details.py:223 and on refresh at125. This digest indexes the local plan cache, including SQLite persisted api_keys_plans.api_key_hash; it is not a stored password verifier. Collector uses the same digest for local SQLite raw/prepared usage routing and maps it back to an already supplied actual key in process memory. Real HTTP authorization uses the actual API key in Bearer headers, not comparison with this hash. Canonical preparation removes api_key_hash from the frozen report. Legacy transport also removes it. The plan cache can influence quota decisions, so the index must avoid collisions; this is not a reason to treat it as password authentication.

The API-key call sites all specify length=-1; current Python slicing returns 63 hex characters (252 bits), not the default five. This off-by-one naming/intent oddity is preexisting and must not be casually changed because existing SQLite key routing/cache entries use the exact bytes. A crypto algorithm/length change would require migration or dual-read compatibility. A password KDF would not improve this routing contract and a randomized salt would break deterministic lookup.

The default five hex characters apply to resource-details fallback identity, hostname, and IP telemetry only. They are 20-bit labels and can collide; resource fallback collisions can combine attribution within the same API-key/category/outcome grouping. Hashing IP/hostname also does not provide strong anonymity for enumerable inputs. These preexisting metadata limitations are distinct from this specific password-hashing alert, whose traced API-key path uses 63 characters. No inference is made here that any digest anonymizes low-entropy input or replaces protection of local files; original credential strings also remain necessary in process memory (and the existing Redis transport intentionally retains original credentials).

Suggested dismissal comment:

“False positive: the traced value is an API key used to derive a deterministic local plan-cache/usage-routing index, not a password verifier. collector.py:726→739 calls PlanDetails.get_api_key_plan; plan_details.py:223 hashes with length=-1 (63 SHA-256 hex characters). HTTP authentication still sends the original supplied API key, and prepared usage strips api_key_hash. Preserve this preexisting digest format for SQLite compatibility; no password-storage/authentication comparison uses this digest.”

Official query documentation distinguishes computationally expensive password hashing from ordinary SHA-256 use: https://codeql.github.com/codeql-query-help/python/py-weak-sensitive-data-hashing/

GitHub documents per-alert dismissal, choosing the correct reason, and retaining an auditable dismissed_comment in PR alert triage: https://docs.github.com/en/code-security/how-tos/manage-security-alerts/manage-code-scanning-alerts/triage-alerts-in-pull-requests#dismissing-an-alert-on-your-pull-request

Do not add a shared-helper suppression comment: I did not verify that Python default CodeQL setup honors that syntax, and it could conceal future real password-verifier uses. Existing repository comments are not proof of analyzer support. The official 2.12.0 AlertSuppression syntax note is in Java/Kotlin improvements, so it does not establish Python support.

Exact-head source links:

• Source and cache call:

  inference/inference/usage_tracking/collector.py

  Lines 722 to 740 in d3f8e74

  	keys = dict(a[::-1] for a in self._hashed_api_keys.items())
  	capabilities = {}
  	enterprises = {}
  	for key in {key for payload in raw for key in payload if key}:
  	api_key = keys.get(key)
  	if not api_key:
  	continue
  	try:
  	capabilities[key] = get_usage_report_capability(
  	api_key,
  	self._settings.api_plan_endpoint_url,
  	ssl_verify=ssl_verify_for_endpoint(
  	self._settings.api_plan_endpoint_url
  	),
  	extra_headers=build_roboflow_api_headers(),
  	)
  	if capabilities[key] is not None:
  	plan = self._plan_details.get_api_key_plan(api_key=api_key)
  	enterprises[key] = plan[self._plan_details._is_enterprise_col_name]

• Cache lookup and refresh:

  inference/inference/usage_tracking/plan_details.py

  Lines 204 to 229 in d3f8e74

  	def get_api_key_plan(
  	self,
  	api_key: APIKey,
  	sqlite_connection: Optional[sqlite3.Connection] = None,
  	date_time_now: Optional[datetime.datetime] = None,
  	) -> Dict[str, Union[str, bool]]:
  	if not api_key:
  	return {
  	self._ts_col_name: self._ts_default,
  	self._api_key_hash_col_name: "",
  	self._is_enterprise_col_name: self._is_enterprise_default,
  	self._is_pro_col_name: self._is_pro_default,
  	self._is_trial_col_name: self._is_trial_default,
  	self._is_billed_col_name: self._is_billed_default,
  	self._over_quota_col_name: self._over_quota_default,
  	}
  	if date_time_now is None:
  	date_time_now = datetime.datetime.now(tz=datetime.timezone.utc)
  	api_key_hash = sha256_hash(api_key, length=-1)
  	if api_key_hash not in self.api_keys_plans:
  	api_key_plan = self.refresh_api_key_plan_cache(
  	api_key=api_key, sqlite_connection=sqlite_connection
  	)
  	else:
  	api_key_plan = self.api_keys_plans[api_key_hash]

• Persisted cache schema:

  inference/inference/usage_tracking/plan_details.py

  Lines 46 to 47 in d3f8e74

  	self._api_key_hash_col_name = "api_key_hash"
  	self._columns[self._api_key_hash_col_name] = "TEXT NOT NULL"

• Digest and frozen-report hash removal:

  inference/inference/usage_tracking/payload_helpers.py

  Lines 248 to 300 in d3f8e74

  	def sha256_hash(payload: str, length=5):
  	payload_hash = hashlib.sha256(payload.encode())
  	return payload_hash.hexdigest()[:length]
  	def get_usage_report_capability(
  	api_key: str,
  	api_plan_endpoint_url: str,
  	ssl_verify: bool = True,
  	extra_headers: Optional[Dict[str, str]] = None,
  	) -> Optional[Dict[str, Any]]:
  	"""A failed lookup is unknown, never permission to downgrade a report."""
  	if OFFLINE_MODE:
  	raise ConnectionError("Offline usage capability lookup")
  	response = requests.get(
  	api_plan_endpoint_url,
  	headers={"Authorization": f"Bearer {api_key}", **(extra_headers or {})},
  	verify=ssl_verify,
  	timeout=1,
  	)
  	response.raise_for_status()
  	body = response.json()
  	if not isinstance(body, dict):
  	raise ValueError("Invalid usage capability response")
  	capability = body.get("usage_report_protocol")
  	if capability is None:
  	return None
  	if (
  	not isinstance(capability, dict)
  	or type(capability.get("version")) is not int
  	or capability.get("version") != 1
  	or not isinstance(capability.get("workspace_id"), str)
  	or not capability["workspace_id"]
  	or not isinstance(capability.get("ownership_fingerprint"), str)
  	or len(capability["ownership_fingerprint"]) != 64
  	or any(c not in "0123456789abcdef" for c in capability["ownership_fingerprint"])
  	):
  	raise ValueError("Invalid usage report capability")
  	return capability
  	def prepare_usage_reports(
  	resource_payloads: ResourceUsage, capability: Dict[str, Any]
  	) -> ResourceUsage:
  	reports = {}
  	for payload in resource_payloads.values():
  	if "processed_frames" not in payload:
  	continue
  	report = deepcopy(payload)
  	report.pop("api_key_hash", None)
  	report.pop("api_key", None)
  	report.pop("_usage_report_candidate", None)
  	report.pop("_legacy_delivery", None)

• Actual credential HTTP authentication:

  inference/inference/usage_tracking/payload_helpers.py

  Lines 321 to 334 in d3f8e74

  	) -> Dict[str, str]:
  	if OFFLINE_MODE or not reports:
  	return {}
  	body = [dict(deepcopy(report), api_key=api_key) for report in reports.values()]
  	try:
  	response = requests.post(
  	api_usage_endpoint_url,
  	json=body,
  	verify=ssl_verify,
  	headers={"Authorization": f"Bearer {api_key}", **(extra_headers or {})},
  	timeout=1,
  	)
  	if response.status_code != 200:
  	return {}

Local cache/usage records remain sensitive data and require their existing filesystem protections. This recommendation assesses the specific password-hashing alert and preserves the prior bounded PR correctness approval; it is not a general security audit or a claim that all telemetry pseudonymization, metadata collision behavior, and local persistence protection are sufficient for every threat model.

Alert477 was subsequently dismissed as false positive with an auditable explanation; source remains unchanged.

[stellasphere]

Dedicated context-isolated simplicity review — APPROVE exact head. Posted by the coordinating agent under the authorized Jarbas-outage substitute; this is not a native GitHub or Jarbas approval.

APPROVE: roboflow/inference PR #2946 at d3f8e74 against 5909dc7.

I reviewed the entire five-file diff and the surrounding collector, payload helpers, SQLite queue, and all new delivery tests. I applied leo-code-comments and its required leo-writing-style guidance. I found no significant simplification that justifies requesting changes.

The original full review covered 855 insertions and 8 deletions, including 482 lines in one focused new test file and one updated existing assertion. The final delta adds a focused credential-order regression and strengthens the revoked-credential scenario. The three production files retain clear responsibilities: the collector negotiates and partitions delivery, helpers freeze and validate the wire protocol, and SQLite owns atomic preparation and acknowledgment. The transaction callback keeps protocol decisions out of SQL without introducing another framework or persistence surface. Small repeated dictionary filtering and table setup do not warrant adding abstractions or files.

The immutable report store is necessary: ordinary raw compaction cannot safely absorb already attempted reports. The candidate marker and sticky legacy marker protect different boundaries, including unmarked persisted usage and ambiguous legacy sends. Removing either to reduce branches would weaken compatibility. Comments are sparse and explain these non-obvious guarantees; I found no verbose or duplicate comment block worth a change request.

Credential replacement adds necessary routing behavior without rewriting a report. The collector snapshots supplied credentials, checks other candidates in reverse insertion order, caches each candidate's workspace lookup within the send pass, and groups transport by credential. Matching a workspace changes only the transport credential; the report ID, totals and ownership fingerprint stay frozen. A global credential registry or persistent capability cache would add invalidation state rather than simplify this patch. The test matrix covers both original-key retention/removal and both memory/SQLite delivery, with a separate unrelated-workspace exclusion case.

The retained-owner check stops repeated preparation for the same original key and ownership fingerprint while outstanding reports remain. It is not a global byte bound: distinct keys or ownership fingerprints can retain separate batches, and existing raw storage is not globally capped by this change. The code and tests support that narrower claim. New usage for another owner can proceed without mutating a conflicted report.

Validation I actually performed: verified the checkout HEAD; read repository AGENTS.md (no CLAUDE.md exists in this checkout); read the complete base-to-head diff, surrounding implementation and all delivery tests; ran git diff --check against the stated base successfully. I did not execute runtime tests in this simplicity pass and do not claim full package CI or real-provider validation. The old handoff was used only for historical scope and rollout boundaries, not as evidence for current-head test results.

This is a human simplicity approval of this exact source head. Public package/image release, receiver readiness and the coupled offloader source pin remain separate rollout gates. No external review was posted and no source, provider, release or deployment state was changed.

Final delta recheck: verified local HEAD d3f8e74 and read the complete diff from 496afa1. The only production change replaces break with continue when the candidate equals the original key. This removes the insertion-order exclusion without adding state or branches. The accompanying test exercises an earlier-known replacement after original revocation in memory and SQLite, checking unchanged report content and total accepted usage. Existing rotation tests now explicitly model revoked original credentials. The two-file delta has 79 insertions and 7 deletions; git diff --check passed. No runtime tests were repeated for this bounded simplicity recheck. APPROVE remains the exact-final-head simplicity verdict.