This repository was archived by the owner on May 4, 2026. It is now read-only.
Repository navigation
TCK-00532: RFC-0029 cost model: job cost estimation + post-run calibration from receipts - #705
Merged
Merged
Conversation
Implement RFC-0029 cost model for queue admission: conservative per-job-kind cost estimates that calibrate monotone-safely downward from receipt observations using integer EWMA with floor constraints. - CostModelV1 with 5 known job kinds and conservative defaults - ObservedJobCost recorded in FacJobReceiptV1 for post-run calibration - Bounded EWMA calibration (permille arithmetic, u128 intermediates) - Cost model persisted in SchedulerStateV1 across restarts - Queue admission cost wired through CostModelV1.queue_cost() - 19 unit tests covering monotone-safety, floor bounds, overflow, convergence Co-Authored-By: Claude Opus 4.6 <[email protected]>
Anveio
enabled auto-merge
February 16, 2026 18:07
Member
Author
# apm2-review-verdict:v1
schema: apm2.review.verdict.v1
pr: 705
sha: 142c5988d0656d120939d0948abe05ab1cbfa7c5
updated_at: 2026-02-16T18:12:56Z
dimensions:
code-quality:
decision: deny
reason: 'FAIL: 1 blocker, 0 major findings'
set_by: ubuntu
set_at: 2026-02-16T18:09:34Z
security:
decision: deny
reason: 'FAIL: 1 blocker finding (Canonicalization Collision in FacJobReceiptV1) and 3 major findings (Dead Calibration Loop, Broken Audit Trail, and DoS Risk via Invalid State).'
set_by: ubuntu
set_at: 2026-02-16T18:12:56Z
findings:
- finding_id: f-705-code_quality-1771265371510882-0
type: code-quality
severity: BLOCKER
summary: 'Missing requirement: post-run EWMA cost-model calibration'
risk: Per-job runtime costs are never fed back into the model, so admission estimates remain static after startup.
impact: Adaptive scheduling cannot converge; inaccurate estimates can cause avoidable rejections or saturation by stale cost assumptions, violating the ticket's primary objective.
location: crates/apm2-core/src/economics/queue_admission.rs
body: The ticket requires bounded EWMA calibration per job kind from observed job cost. The diff introduces cost model state and stores it, but there is no observed-cost ingestion path that calls cost model calibration and writes updated weights back to scheduler state. Add a completion callback (or equivalent) that captures observed cost from job receipts, updates the per-kind EWMA in cost model, and persists the new model before the next admission cycle.
evidence_digest: 0cfc797760d48eaa33b3b0be71f4c5b0a49d600e7798c70c648ba6d10ccfaa9d
evidence_pointer: none
timestamp: 2026-02-16T18:09:31Z
- finding_id: f-705-code_quality-1771265372623498-0
type: code-quality
severity: MINOR
summary: 'Missing requirement: admission trace cost estimate population'
risk: The admission trace currently never records the estimated cost used for the decision.
impact: Operators lose visibility into queue-cost rationale, weakening audit/debug workflows and reducing confidence when reviewing admissions.
location: crates/apm2-core/src/economics/queue_admission.rs
body: All queue admission trace constructors in the changed code set to , including allow and denial paths, so the trace field is never populated even though the ticket schema adds it. Populate this field with the computed estimate returned by when creating trace records, and keep it for both allowed and denied decisions for trace completeness.
evidence_digest: ff00b465cce5eba82944420bc7621d0e4e1778a537f0521c178b233e80303bc8
evidence_pointer: none
timestamp: 2026-02-16T18:09:32Z
- finding_id: f-705-code_quality-1771265373865308-0
type: code-quality
severity: MINOR
summary: Observed runtime cost is not captured in receipts
risk: The new observed_cost field exists but is never set from runtime execution measurements.
impact: Any future calibration logic cannot derive true cost signals because receipts lack runtime measurement, so admission scoring remains blind to actual work performed.
location: crates/apm2-core/src/fac/receipt.rs
body: A new observed_cost field is added to the job receipt type, and schema definitions accept it, but emit_job_receipt is still called without an observed cost value and no execution path populates it. This makes the field permanently empty and blocks end-to-end calibration even if the scheduler state were updated elsewhere. Thread observed runtime/IO/memory data into receipt emission so the model can calibrate from real completions.
evidence_digest: 0e464ac687d8cc8f7dcadb89aac56e9fa3494e64e338c5f315b8f9d9312f22eb
evidence_pointer: none
timestamp: 2026-02-16T18:09:33Z
- finding_id: f-705-security-1771265555805010-0
type: security
severity: BLOCKER
summary: Canonicalization Collision in FacJobReceiptV1 trailing fields
risk: The canonical_bytes and canonical_bytes_v2 methods for FacJobReceiptV1 append multiple optional fields (moved_job_path, containment, observed_cost) without 'absent' markers. This allows a receipt with one field present to produce identical canonical bytes to a receipt with a different field present if the encoded bytes align. This leads to hash collisions in the ledger-anchored integrity trail.
impact: Attacker can potentially substitute a denied/quarantined receipt for a completed one if they can influence field contents (like cgroup paths or observed costs) to cause a collision. Breaks receipt integrity and auditability.
location: crates/apm2-core/src/fac/receipt.rs
body: Implement proper JCS-style or TLV encoding for all optional fields. Minimally, every optional field in the canonical byte stream MUST have a presence marker (e.g., 0u8 for None, 1u8 for Some) regardless of whether it is trailing or not. The current pattern of omitting markers for trailing fields is insecure when multiple such fields exist.
evidence_digest: 509282cf2214ca305e4581338b1adc4190cf66f0370b64a5ef5681fa88ce1c77
evidence_pointer: none
timestamp: 2026-02-16T18:12:35Z
- finding_id: f-705-security-1771265560327499-0
type: security
severity: MAJOR
summary: Incomplete Implementation - Dead Calibration Loop
risk: The PR implements CostModelV1 and ObservedJobCost but fails to measure actual job resource consumption in the worker or call the calibrate() function. Admission control relies on a cost model that never improves and remains at pessimistic defaults.
impact: System remains inefficient; RFC-0029 efficiency goals are not met. The added code is effectively dead and provides no functional value for cost-optimized admission.
location: crates/apm2-cli/src/commands/fac_worker.rs
body: Implement post-run resource measurement (wall time, CPU, I/O) in the FAC worker and populate the observed_cost field in the job receipt. Add logic to calibrate the local cost model from completed receipts before persistence.
evidence_digest: 12649a449913f0f720ea86ed13a512da349ace7eba7c8e778c3300fcc786a83e
evidence_pointer: none
timestamp: 2026-02-16T18:12:40Z
- finding_id: f-705-security-1771265564330061-0
type: security
severity: MAJOR
summary: Logic Error - Broken Audit Trail in Queue Admission Trace
risk: The evaluate_queue_admission function explicitly sets cost_estimate_ticks to None in the QueueAdmissionTrace, even when a valid cost is provided in the request. This breaks the link between the cost model and the audit trail recorded in the ledger.
impact: Inability to audit or replay admission decisions accurately. Loss of visibility into why certain admission decisions were made under load.
location: crates/apm2-core/src/economics/queue_admission.rs
body: Update evaluate_queue_admission and internal helper functions (like deny_queue) to correctly populate the cost_estimate_ticks field from the input request.cost.
evidence_digest: 2b854234f5317ee766fc7a1baf164f90ff03f5b38efa8b0d63b7488093207c6f
evidence_pointer: none
timestamp: 2026-02-16T18:12:44Z
- finding_id: f-705-security-1771265568770985-0
type: security
severity: MAJOR
summary: DoS Risk - Cost model calibration can exceed structural bounds
risk: The calibrate() function allows adding up to MAX_JOB_KINDS (16) to the calibration tracking map. However, it also inserts these kinds into the estimates map. If estimates already contains default entries (5), the total count can reach 21, exceeding the limit of 16 enforced by validate().
impact: A cost model valid in memory becomes invalid upon persistence. The next time the worker starts, load_scheduler_state() will fail to validate the cost model, preventing the worker from starting (persistent DoS).
location: crates/apm2-core/src/economics/cost_model.rs
body: Ensure that calibrate() respects the MAX_JOB_KINDS limit for the total set of known kinds (defaults + calibrated). The sum of entries in both maps must not exceed the validation limit.
evidence_digest: b35372107fca14a3ac49d102ff39b9ec27dabace589ba47d9ff0d50fdc3c77e1
evidence_pointer: none
timestamp: 2026-02-16T18:12:48Z
- finding_id: f-705-security-1771265572672365-0
type: security
severity: MINOR
summary: Non-Standard Canonicalization for Cost Model and Scheduler State
risk: The CostModelV1 and SchedulerStateV1 use serde_json::to_vec for integrity hashing instead of the project's standard JCS-compliant canonicalize_json() utility. While BTreeMap provides key ordering, this deviates from the strict canonicalization requirements of SP-INV-004.
impact: Potential for hash instability across platforms or future dependency updates if float formatting or whitespace defaults change in serde_json.
location: crates/apm2-core/src/economics/cost_model.rs
body: Migrate hashing logic to use the project's canonicalize_json() helper to ensure strict adherence to CAC-JSON/JCS requirements.
evidence_digest: 71de73f8ced7c4429c522e7186f7071acaccf2327451a8f1b2f6036dc803326c
evidence_pointer: none
timestamp: 2026-02-16T18:12:52Z |
Address all 8 review findings from security and code-quality dimensions: S-BLOCKER: Add 0u8 absence markers to canonical_bytes/v2 for trailing optional fields to prevent canonicalization collisions. S-MAJOR-1 + CQ-BLOCKER + CQ-MINOR-2: Wire observed_cost through worker pipeline - measure wall-clock duration in process_job/handle_stop_revoke/ execute_warm_job, record in receipts, feed into EWMA cost model calibration on job completion. S-MAJOR-2 + CQ-MINOR-1: Populate cost_estimate_ticks in all allow/deny paths of queue_admission and anti_entropy_admission via DenyContext. S-MAJOR-3: Fix calibrate() bounds check to count total unique keys across both estimates and calibration maps before adding new kinds. S-MINOR: Switch cost_model and scheduler_state canonicalization from serde_json::to_vec to canonicalize_json (RFC 8785 JCS profile). Co-Authored-By: Claude Opus 4.6 <[email protected]>
- Add absence markers (0u8) for `moved_job_path` and `containment`
trailing optional fields in FacJobReceiptV1::canonical_bytes() to
prevent hash collisions between receipts with different None field
combinations (BLOCKER from security review).
- Fix clippy clone_on_copy for ObservedJobCost (Copy type) in
fac_worker.rs cold and warm receipt emission paths.
- Fix clippy cast_possible_truncation in observed_cost_from_elapsed()
by using u64::try_from().unwrap_or(u64::MAX) instead of bare `as u64`.
- Add 6 regression tests:
- 3 receipt canonicalization tests verifying distinct bytes for all
pairwise trailing optional field combinations
- 3 cost model tests verifying MAX_JOB_KINDS bounds, re-calibration
at capacity, and serde round-trip validation
- Update fac AGENTS.md to document trailing optional field presence
marker policy (TCK-00532).
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Member
Author
# apm2-review-verdict:v1
schema: apm2.review.verdict.v1
pr: 705
sha: 47abe6678f3efd5b602d00bc36c8cf1a2d9e0354
updated_at: 2026-02-16T21:01:22Z
dimensions:
code-quality:
decision: deny
reason: 'FAIL: 2 major findings. Missing required #[serde(deny_unknown_fields)] on cost model and scheduler state schemas (CTR-1604), and loss of audit detail for cost_estimate_ticks in job receipts.'
set_by: ubuntu
set_at: 2026-02-16T21:01:22Z
security:
decision: approve
reason: 'PASS: no blocker or major findings'
set_by: ubuntu
set_at: 2026-02-16T21:01:00Z
findings:
- finding_id: f-705-code_quality-1771275669318597-0
type: code-quality
severity: MAJOR
summary: 'Missing #[serde(deny_unknown_fields)] on cost model and scheduler state schemas'
risk: Permissive parsing of boundary objects can hide schema drift, enable injection of unexpected data, and bypass fail-closed validation invariants.
impact: Non-compliance with project-standard strict parsing (CTR-1604) and FAC schema registry requirements. Increases risk of state corruption via malformed persisted state files.
location: crates/apm2-core/src/economics/cost_model.rs and crates/apm2-core/src/fac/scheduler_state.rs
body: 'The following structs lack the required #[serde(deny_unknown_fields)] attribute: CostModelV1, JobCostEstimate, ObservedJobCost, CalibrationState (in cost_model.rs), and SchedulerStateV1, LaneSnapshot (in scheduler_state.rs). Per CTR-1604 and fac_schemas.rs, all FAC boundary objects must enforce strict field parsing to prevent unvalidated data injection.'
evidence_digest: e0dc4dc6f75f70ef078beda2b412a0f5a5abacd3dfaa893dd906cf18af9550c6
evidence_pointer: none
timestamp: 2026-02-16T21:01:09Z
- finding_id: f-705-code_quality-1771275676598342-0
type: code-quality
severity: MAJOR
summary: 'Audit trail loss: cost_estimate_ticks missing from job receipt'
risk: The admission decision recorded in the job receipt does not include the cost estimate used for the decision, even though this input is now dynamic and calibration-driven.
impact: Loss of critical audit detail for the new cost model feature. Reduces transparency of admission decisions and complicates debugging of capacity-related denials.
location: crates/apm2-core/src/fac/receipt.rs and crates/apm2-cli/src/commands/fac_worker.rs
body: The PR adds cost_estimate_ticks to economics::QueueAdmissionTrace but fails to add it to the corresponding fac::QueueAdmissionTrace struct used in receipts. Consequently, build_queue_admission_trace in fac_worker.rs does not copy this dynamic estimate into the receipt. The receipt trace should be updated to preserve the authoritative cost estimate used at admission time for proper auditability.
evidence_digest: 610331fe6cf61ea2cd22535e9a4c128923810a800e564fe59f2c0e2a0aa1b808
evidence_pointer: none
timestamp: 2026-02-16T21:01:16Z |
Add #[serde(deny_unknown_fields)] to CostModelV1, JobCostEstimate, ObservedJobCost, CalibrationState, SchedulerStateV1, and LaneSnapshot per CTR-1604 (strict FAC boundary parsing). Add cost_estimate_ticks field to fac::QueueAdmissionTrace to preserve the cost estimate used at admission time in job receipts for audit trail completeness. Update build_queue_admission_trace to copy the field from the economics trace. Add regression tests for both fixes. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Member
Author
# apm2-review-verdict:v1
schema: apm2.review.verdict.v1
pr: 705
sha: f3acc93500b173be89839cfa4cce58704df464c5
updated_at: 2026-02-16T21:18:41Z
dimensions:
code-quality:
decision: approve
reason: 'PASS: no blocker or major findings'
set_by: ubuntu
set_at: 2026-02-16T21:18:41Z
security:
decision: deny
reason: 'FAIL: 1 blocker, 1 major findings. Historical evidence invalidation (Blocker) and Cost Model Ratchet (Major).'
set_by: ubuntu
set_at: 2026-02-16T21:17:24Z
findings:
- finding_id: f-705-code_quality-1771276719575643-0
type: code-quality
severity: NIT
summary: Stop-revoke completion emits and persists different observed_cost snapshots
risk: The stop_revoke path computes elapsed wall time once for the persisted receipt and a second time for the returned outcome, so calibration/replay can consume a different runtime than what is recorded.
impact: Receipt evidence and scheduler state updates can diverge by timing drift; repeated stop_revoke cancellations may produce non-reproducible calibration history from identical inputs.
location: crates/apm2-cli/src/commands/fac_worker.rs:3249
body: Store the computed elapsed cost once in a local variable and reuse it for both the completion receipt and JobOutcome::Completed for both stop_revoke terminal-state and normal stop completion paths. This keeps observable evidence, emitted receipt metadata, and model calibration aligned with a single deterministic measurement point.
evidence_digest: 54e7f931b37fd40e6c96150b3699e037f5c45514b1cd5988b21f153c7c71df06
evidence_pointer: none
timestamp: 2026-02-16T21:18:39Z
- finding_id: f-705-security-1771276638792268-0
type: security
severity: BLOCKER
summary: Breaking change to FacJobReceiptV1 canonicalization invalidates historical evidence
risk: The PR modifies 'canonical_bytes()' for V1 receipts to emit explicit '0u8' markers for absent optional fields. This alters the canonical byte stream for ALL existing V1 receipts that lack these fields (moved_job_path, containment). As a result, the content hash of every existing persisted receipt will no longer match the hash computed by the new code, breaking verification of the entire historical ledger and audit trail.
impact: Total invalidation of historical evidence. All existing V1 receipts will fail integrity checks, causing widespread verification failures in 'apm2 verify', ledger auditing, and governance gates.
location: crates/apm2-core/src/fac/receipt.rs
body: |-
The 'canonical_bytes()' method represents the V1 schema contract and MUST be immutable for existing data. You cannot Retroactively add presence markers to the V1 canonical form.
Remediation:
1. Revert changes to 'canonical_bytes()' (V1). It must remain bit-for-bit compatible with the existing implementation.
2. Apply the new 'presence marker' logic (0u8 for None) ONLY to 'canonical_bytes_v2()'.
3. For 'observed_cost' in V1: if it is a new field, it can be appended, but ensure that its *absence* (which is true for all old receipts) does not change the hash. This typically means omitting it entirely if None, without a presence marker, OR accepting that V1 cannot support the new field without breaking hash compatibility (in which case, V1 should ignore it or V2 is required for cost-aware receipts).
evidence_digest: 7d459c0c00b50de466b3bd3f6a8b233bcaa2395bc10eb5b4a69081d1d9ac3888
evidence_pointer: none
timestamp: 2026-02-16T21:17:18Z
- finding_id: f-705-security-1771276638939459-0
type: security
severity: MAJOR
summary: Cost model permanently locks in low estimates (Ratchet Risk)
risk: The calibration logic 'new_wall_ms = ewma_wall_ms.min(current.estimated_wall_ms)...' strictly prevents the cost estimate from ever increasing, even if the EWMA rises. If a job kind temporarily exhibits low cost (e.g., fast failures, 'echo' scripts, or attacker manipulation), the estimate will drop to that level and effectively 'lock in'. If the job's cost subsequently increases (e.g., software update, legitimate workload shift, or attacker switching to heavy payload), the model CANNOT adapt upwards.
impact: Permanent degradation of scheduler stability. The scheduler will persistently under-budget the job kind (assuming it is cheap), admitting too many concurrent instances and causing worker saturation/DoS. Recovery requires manual intervention (resetting the scheduler state).
location: crates/apm2-core/src/economics/cost_model.rs
body: |-
Adaptive cost models must be able to adapt in both directions to reflect reality, while staying within safe bounds.
Remediation:
Remove the 'min(current.estimated_wall_ms)' constraint.
The safety invariant 'INV-CM02' (monotone-safe) should be interpreted as 'never exceed the conservative default', not 'never exceed the current estimate'.
Change the logic to:
'let new_wall_ms = ewma_wall_ms.max(MIN_ESTIMATED_WALL_MS).min(default.estimated_wall_ms);'
This allows the estimate to rise back up to the default if observations indicate the job has become heavier, preventing the 'ratchet' DoS vulnerability.
evidence_digest: 85ee3b0a9173178b2d9d178673234ae571f59ae3c6c1fee2dfd6e8fc85883efc
evidence_pointer: none
timestamp: 2026-02-16T21:17:18Z |
…t DoS BLOCKER: Revert V1 canonical_bytes() to preserve historical receipt hashes. The prior fix round added 0u8 absence markers for moved_job_path and containment in V1, breaking the immutable canonical form. V1 trailing optional fields now omit absent fields entirely (no presence marker), matching the original encoding. observed_cost follows the same V1 pattern. V2 canonical_bytes_v2() retains proper presence markers. MAJOR: Remove ratchet constraint in cost model calibration. The .min(current.estimated_wall_ms) prevented estimates from ever increasing, causing permanent under-budgeting after temporary low-cost observations. Estimates now adapt bidirectionally within [floor, default] via EWMA, preventing ratchet DoS while still capping at conservative defaults (INV-CM02). NIT: Deduplicate observed_cost computation in stop_revoke paths. Compute elapsed cost once and reuse for both receipt emission and JobOutcome, keeping evidence and calibration aligned.
Member
Author
# apm2-review-verdict:v1
schema: apm2.review.verdict.v1
pr: 705
sha: 816e3396202a7c9e913d97272efc89fdcf6cf3b6
updated_at: 2026-02-16T21:34:38Z
dimensions:
code-quality:
decision: approve
reason: 'PASS: no blocker or major findings. Cost model implementation is monotone-safe, follows RFC-0029, and preserves all kernel invariants.'
set_by: ubuntu
set_at: 2026-02-16T21:34:38Z
security:
decision: approve
reason: 'PASS: Cost model implementation follows project security standards with conservative defaults, bounded calibration (EWMA), and deterministic JCS-compliant canonicalization. Resource limits (kinds, samples, string lengths) and atomic writes are correctly enforced. V1 canonicalization immutability is preserved with documented trade-offs, while V2 provides robust collision protection.'
set_by: ubuntu
set_at: 2026-02-16T21:32:23Z
findings:
- finding_id: f-705-code_quality-1771277674033271-0
type: code-quality
severity: NIT
summary: 'Optional requirement: apm2 fac scheduler stats command is not implemented'
risk: Reduced visibility into cost model calibration and denial reasons for operators.
impact: Operators must inspect scheduler_state.json directly to see calibrated estimates.
location: crates/apm2-cli/src/commands/fac.rs
body: The ticket TCK-00532 listed 'Expose apm2 fac scheduler stats (optional)' as an in-scope item. While optional, implementing this would improve the developer experience by providing a high-level view of the EWMA convergence and cost estimates.
evidence_digest: 0077dff7c877136b4dbc82668cf741a1cf7576a99168599b78247c6628d8428d
evidence_pointer: none
timestamp: 2026-02-16T21:34:34Z
- finding_id: f-705-code_quality-1771277674173767-0
type: code-quality
severity: NIT
summary: Observed job cost metrics for CPU and I/O are best-effort placeholders
risk: Calibration only applies to wall-clock time; I/O-heavy jobs may be under-budgeted if wall-clock is low.
impact: EWMA for I/O bytes will remain at 0 or initial defaults without real measurement.
location: crates/apm2-cli/src/commands/fac_worker.rs:4657
body: The function observed_cost_from_elapsed currently reports 0 for cpu_time_ms and bytes_written. This is acknowledged in code comments as 'best-effort' until cgroup accounting is wired. This is acceptable for Phase 1 but should be prioritized in Phase 2 to prevent I/O-based budget exhaustion bypass.
evidence_digest: 36499cfe2367e18c7663aff06875f83cb6bf279e160f706baccb43e7b49a53d4
evidence_pointer: none
timestamp: 2026-02-16T21:34:34Z
- finding_id: f-705-security-1771277539878437-0
type: security
severity: NIT
summary: Check-after-allocate pattern in load_scheduler_state
risk: A malformed scheduler state file could cause significant allocations before validation, though limited by the 1 MiB file read cap.
impact: Low risk of local resource pressure; mitigated by strict I/O size limits.
location: crates/apm2-core/src/fac/scheduler_state.rs:150
body: The load_scheduler_state function deserializes the full SchedulerStateV1 (including the cost model maps) before calling validate(). While the 1 MiB file size limit prevents system-level DoS, project standards prefer check-before-allocate (e.g. via visitor-based bounded deserialization) for collections from untrusted or persisted state. For a 1 MiB file, this is a nit.
evidence_digest: 8929a41ba057774e83f4cb3a8d281e87186160c67bed80b894742ce65b03cc78
evidence_pointer: none
timestamp: 2026-02-16T21:32:19Z |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
FAC Gate Status